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: unnecessary lowercasing parameters #2877

Merged
merged 1 commit into from Jan 26, 2024

Conversation

bitfactory-robin-martijn
Copy link
Contributor

Related:

Issue

When using URLHelper::get_params(), everything is normalised to lowercase. I understand why you would do that with URLs, but in this specific instance it is about parameters. I think the author of the issue raises a valid point that case sensitive parameters are valid. The RFC only speaks of normalising the protocol and scheme: https://www.rfc-editor.org/rfc/rfc3986#page-11

Solution

I removed the strtolower. (And fixed a small typo earlier in the file)

Impact

If some code is relying on this method also turning everything into lowercase for them, that would break.

Usage Changes

Developers should put strtolower around the output if they need that.

Considerations

I realise it is a breaking change, and therefore not ideal. I am however convinced that the original solution is the actual breaking thing here.

Testing

All existing tests were already lowercase.

Copy link
Member

@Levdbas Levdbas left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this and the typo along the way @expedition-robin-martijn

Copy link
Member

@nlemoine nlemoine left a comment

Choose a reason for hiding this comment

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

Still have to figure out what this URLHelper was made for ^^

In the meantime, this is legitimate 👍

@nlemoine nlemoine merged commit 664ea62 into timber:2.x Jan 26, 2024
8 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants