Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ResponseInterface (2) #6569

Merged
merged 13 commits into from
Oct 9, 2022
Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Sep 23, 2022

Needs to rebase after merging #6556

Description
Ref #4356

Now the framework depends on ResponseInterface, not Response.

Add

  • Response::getCSP()
  • ResponseInterface::getCSP()
  • ResponseInterface::getReasonPhrase()
  • ResponseInterface::getCookieStore()

Change APIs

  • API\ResponseTrait::failServerError()
  • Services::exceptions()
  • Debug\Exceptions::__construct()

Deprecate

  • Response::$CSP. It is now public, but should be protected.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities 4.3 labels Sep 23, 2022
@kenjis kenjis marked this pull request as draft September 23, 2022 01:24
@kenjis kenjis force-pushed the fix-ResponseInterface-2 branch 2 times, most recently from ef27f71 to 7e01a7e Compare September 23, 2022 01:53
@MGatner
Copy link
Member

MGatner commented Sep 23, 2022

Awaiting rebase for this one.

@kenjis kenjis force-pushed the fix-ResponseInterface-2 branch from 82beab6 to f110599 Compare September 23, 2022 23:50
@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Sep 23, 2022
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the plan was to release the ValidationInterface changes in 4.3 and then consider these others for 4.4. Have you changed your opinion?

(Edit: code looks good BTW!)

@kenjis
Copy link
Member Author

kenjis commented Sep 24, 2022

I didn't think of 4.3 strictly as Validation only.
If you don't want to include any more interface changes in 4.3, this PR can be moved to 4.4.

@kenjis
Copy link
Member Author

kenjis commented Sep 24, 2022

I really want to fix Request as well, but it looks like it will require even bigger changes.
See #6573

@MGatner
Copy link
Member

MGatner commented Sep 25, 2022

Well when we originally talked about that I thought 4.3 would be out by now. I've been holding for @sclubricants's awesome database features, but maybe we should do 4.2.7 and 4.3 now and then spin up 4.4 for the database features and Request changes. Nothing wrong with having two minor releases close to each other. Thoughts from either of you?

@kenjis
Copy link
Member Author

kenjis commented Sep 25, 2022

4.2.7 is ready.

Speaking of 4.3, #6536 is almost complete and all that remains is the documentation.
But @sclubricants's work continues. Can't wait for all.
So we may as well release 4.3 now.

@sclubricants
Copy link
Member

sclubricants commented Sep 25, 2022

Once we get #6536 merged then It should take long to add the upsert feature. The code is already written.. tests and all. Once the refactor is complete is should be as simple as adding the upsert methods. I have plans to add more features but those can wait for a later release. It would be nice to get the upsert feature into 4.3. At least if developers have to go through the pain of all the changes that have taken place it would be nice for them to have a new feature to work with.

Also, if we can get #6552 merged then I'd like to add the final piece of the Forge changes which is a method processIndexes(). This will allow creating indexes after table creation. All the code is written for this as well.

If we focus on getting these changes merged I would think we could have them completed in a week or two. Not that we need to rush things but most of the breaking changes have already been made so the rest should go quicker.

I updated #6536 so hopefully it is ready to merge now and I can get the upsert PR submitted.

@MGatner
Copy link
Member

MGatner commented Sep 26, 2022

Thanks for the updates @sclubricants! I'm mostly just reading comments on these and letting the database team handle reviews but let me know if I can help more. Let's aim for 4.3 this coming weekend and if it looks like some of your desired merges need a little more time then I am fine holding. I also am really fine with 4.4 coming right on the heels of 4.3, through.

@kenjis kenjis force-pushed the fix-ResponseInterface-2 branch from f110599 to d9e5919 Compare October 8, 2022 09:57
@kenjis kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Oct 8, 2022
@kenjis kenjis marked this pull request as ready for review October 8, 2022 23:43
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we go 🤞

@kenjis kenjis merged commit f8443a2 into codeigniter4:4.3 Oct 9, 2022
@kenjis kenjis deleted the fix-ResponseInterface-2 branch October 9, 2022 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants