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

[8.x] Change whereStartsWith, DocBlock to reflect that array is supported #39370

Merged
merged 3 commits into from
Oct 26, 2021

Conversation

tanthammar
Copy link
Contributor

A non-breaking change to avoid IDE warnings

whereStartsWith, whereDoesntStartWith and thatStartWith DocBlocks says it only accepts a string, but an array also works, as it's being passed to Str::startsWith($haystack, $needles) which accepts an array too.

I also changed the variable name from $string to $needles for better code readability.
That could potentially be a breaking change if php8 users have started to use named properties...

Would suggest that official documentation is updated to help others save time.

User benefits:

Due to my IDE and official docs, I thought I had to do this:

<input  {{ $attributes->whereDoesntStartWith('wire:model')->whereDoesntStartWith('x-model') }}  />

When I discovered that this is supported:

<input  {{ $attributes->whereDoesntStartWith(['wire:model', 'x-model']) }}  />

Creds to @ryangjchandler who told me :)

whereStartsWith, whereDoesntStartWith, thatStartWith DocBlocks says it only accepts a string, but an array is accepted as it's being passed to Str::startsWith($haystack, $needles) which accepts an array too.
@taylorotwell
Copy link
Member

Tests are failing on this one.

@morloderex
Copy link
Contributor

@tanthammar Looks like you didn't change the use variable in the closures.

@GrahamCampbell GrahamCampbell changed the title Change whereStartsWith, DocBlock to reflect that array is supported [8.x] Change whereStartsWith, DocBlock to reflect that array is supported Oct 26, 2021
@tanthammar
Copy link
Contributor Author

@tanthammar Looks like you didn't change the use variable in the closures.

@morloderex so true, thank you :)

@ryangjchandler
Copy link
Contributor

I don't think you can change the parameter names on a minor release, would need to target 9.x / master or keep the names the same. Correct me if I'm wrong though.

@tanthammar
Copy link
Contributor Author

I don't think you can change the parameter names on a minor release, would need to target 9.x / master or keep the names the same. Correct me if I'm wrong though.

I don't mind changing it.
Didn't read anywhere that Laravel framework has "locked" its parameter names yet?

Guess @taylorotwell has the last say in the matter.

I'll gladly change it back, that part is not important, just better to read.

@driesvints
Copy link
Member

I don't think you can change the parameter names on a minor release, would need to target 9.x / master or keep the names the same. Correct me if I'm wrong though.

@ryangjchandler we don't consider named arguments part of the public API: https://laravel.com/docs/8.x/releases#named-arguments

@taylorotwell taylorotwell merged commit 2239b02 into laravel:8.x Oct 26, 2021
@ryangjchandler
Copy link
Contributor

Thanks for clarifying @driesvints 🤗

@tanthammar tanthammar deleted the patch-1 branch October 27, 2021 14:49
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.

5 participants