Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

revert: #55939 #55943

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

Merged
merged 1 commit into from
Jun 6, 2025
Merged

revert: #55939 #55943

merged 1 commit into from
Jun 6, 2025

Conversation

NickSdot
Copy link
Contributor

@NickSdot NickSdot commented Jun 6, 2025

revert: #55939

The first cast is never reached, because ! str_contains($key, '::') would throw on $key = null .
The other both casts would prevent PHP 8.4 from throwing on a real error (and are also never reached when called from within the class).
$key should never be null here; it can only result in an undesired outcome.

The author of the PR didn't explain what their issue is. The merged PR is, however, the wrong fix.

cc: @Khuthaily

@NickSdot NickSdot changed the title revert: https://github.com/laravel/framework/pull/55939 revert: #55939 Jun 6, 2025
@donnysim
Copy link
Contributor

donnysim commented Jun 6, 2025

I agree with the revert, the function clearly states it only accepts string, if at any point a null value is received, it's not this function that needs to "handle" it but the one calling it up somewhere up the chain.

@NickSdot
Copy link
Contributor Author

NickSdot commented Jun 6, 2025

The only way this could ever do something is when the class was extended and parseNamespacedSegments() is called from a child. But then it would result in $key on the target array to be an empty string and the value array:1 [ 0 => ""].

In all other situations, str_contains throws before.

@taylorotwell taylorotwell merged commit f97e6d9 into laravel:12.x Jun 6, 2025
61 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.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.