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: remove deprecated upper functionality in Request::getMethod() #8186

Merged
merged 24 commits into from
Nov 19, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 10, 2023

Description
The strike line is annoying.
Screenshot 2023-11-10 13 55 00

  • remove deprecated upper functionality in getMethod()
  • fix HTTP verbs case. E.g., getGET.

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 refactor Pull requests that refactor code breaking change Pull requests that may break existing functionalities 4.5 labels Nov 10, 2023
@kenjis kenjis marked this pull request as draft November 10, 2023 01:17
@kenjis kenjis marked this pull request as ready for review November 10, 2023 02:48
@kenjis kenjis marked this pull request as draft November 10, 2023 02:57
@kenjis kenjis marked this pull request as ready for review November 10, 2023 04:19
@kenjis kenjis marked this pull request as draft November 10, 2023 04:21
@kenjis kenjis marked this pull request as ready for review November 10, 2023 04:58
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

I am a little worried about edge cases that don't conform to the HTTP specification. However, I don't know how common it is... probably very rare.

This is a big change though.

@kenjis
Copy link
Member Author

kenjis commented Nov 10, 2023

Yes, this is a big change.
But it has been deprecated since v4.0.5 (January 31, 2021):
https://codeigniter4.github.io/CodeIgniter4/installation/upgrade_405.html?highlight=getmethod#message-getheader-s

@kenjis
Copy link
Member Author

kenjis commented Nov 10, 2023

I found this: JakeChampion/fetch#254
It seems in fetch API only PATCH is not normalized, so patch does not work.

@kenjis
Copy link
Member Author

kenjis commented Nov 10, 2023

In the routes array, there is still lower case method names.
We need to change them.

@michalsn
Copy link
Member

I'm more worried about people using

$this->request->getMethod() === 'post'

@kenjis
Copy link
Member Author

kenjis commented Nov 11, 2023

I understand your concern. But it has been documented for long time.
https://codeigniter4.github.io/CodeIgniter4/incoming/incomingrequest.html#getmethod

We must change the output of getMethod() at once. If it is not changed, almost nothing will change. If we add a feature flag, we will have to prepare more than twice as much test code.

In the end, the question is when to fix this bug. If it is still too early, we can postpone, but what can we do in the meantime?

@michalsn
Copy link
Member

I get it, believe me. I have no problem with removing depreciated code in general. Most such code is long gone and/or smoothly replaced by alternatives.

Here, for a long time we had no alternatives - until the is() method was implemented. That's why I think this could be a pretty painful change.

However, looking at the amount of removed depreciated code in the upcoming 4.5 - maybe it's the right time.

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Let's see what others say.

system/HTTP/OutgoingRequest.php Show resolved Hide resolved
Comment on lines 507 to 516
elseif (array_key_exists(strtolower($method), $this->config->methods)) {
$found = true;
$method = strtolower($method);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we trigger an E_USER_DEPRECATED here when this elseif branch is executed so that users will be aware?

Copy link
Member

Choose a reason for hiding this comment

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

I like that

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

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.

This is a big one. While we're doing this a couple of thoughts to consider if there is a case to make this part of something even bigger:

  1. Enums. Should our HTTP verbs be an Enum framework-wide to decrease future issues and make static analysis easier? I could also see a case for class constants.
  2. PSR-7. Is it time to make the few breaking changes necessary to make our HTTP layer compatible with the PSR? Instead of making this one breaking change and then others in the future it might be good to handle it all at once.

* @TODO For backward compatibility. Remove strtolower() in the future.
* @deprecated 4.5.0
*/
$method = strtolower($route[0]);
Copy link
Member

Choose a reason for hiding this comment

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

If you go with @paulbalandan's trigger idea this would be another good place for a conditional warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@kenjis
Copy link
Member Author

kenjis commented Nov 19, 2023

@MGatner The getMethod() returns string, and our input to HTTP methods is all string.
So it seems difficult to use enum. I added a class with constants.

@kenjis kenjis force-pushed the fix-HTTP-verb-cases branch 2 times, most recently from 42d89a2 to fdb1d81 Compare November 19, 2023 10:15
@kenjis kenjis marked this pull request as draft November 19, 2023 11:57
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.

Thanks, new class looks good. We can revisit PSR-7 as needed before 4.5 goes live.

@kenjis kenjis marked this pull request as ready for review November 19, 2023 12:51
@kenjis kenjis merged commit 4a5cb90 into codeigniter4:4.5 Nov 19, 2023
46 checks passed
@kenjis kenjis deleted the fix-HTTP-verb-cases branch November 19, 2023 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.5 breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants