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 $collection->group() for case-sensitive #5631

Merged
merged 1 commit into from
Sep 16, 2023

Conversation

distantnative
Copy link
Member

This PR …

Fixes

Ready?

  • Unit tests for fixed bug/feature
  • In-code documentation (wherever needed)
  • Tests and checks all pass

For review team

@distantnative distantnative added this to the 3.9.7 milestone Sep 14, 2023
@distantnative distantnative requested a review from a team September 14, 2023 17:28
@distantnative distantnative self-assigned this Sep 14, 2023
@distantnative distantnative changed the base branch from v4/develop to develop September 14, 2023 17:29
@distantnative distantnative linked an issue Sep 14, 2023 that may be closed by this pull request
@afbora
Copy link
Member

afbora commented Sep 14, 2023

Why is the parameter name $i?. I spent a while trying to figure out what $i is for. How about a better variable name?

@lukasbestle
Copy link
Member

What about $ignoreCase? It would be a breaking change with PHP named params, but I agree it would be very worth it.

@afbora
Copy link
Member

afbora commented Sep 15, 2023

Or may be bool $caseSensitive = false

@distantnative
Copy link
Member Author

@afbora But then it wouldn't be just a breaking change of the parameter name but all existing calls would do exactly the opposite of what they did before

@distantnative
Copy link
Member Author

distantnative commented Sep 16, 2023

It gets even messier. Collection has a $caseSensitive class prop. Though this seems to be only used in one test. I am wondering if we should deprecate that to get a bit more clarity.

On the parameter name, I'd go with $caseInsensitive as that's used throughout the Str class.

@distantnative
Copy link
Member Author

distantnative commented Sep 16, 2023

Ok, revert... if we rename this parameter, this fix can't be in 3.9.7.

I suggest we merge the fix without the renamed parameter and rename the parameter for v4: #5634

@distantnative distantnative merged commit 2ab3c90 into develop Sep 16, 2023
@distantnative distantnative deleted the fix/5628-collection-group branch September 16, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Collection->group() throws error with $i set to false
3 participants