Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[12.x] Fix URL generation with optional parameters (regression in #54811) #55213

Merged
merged 3 commits into from
Mar 30, 2025

Conversation

stancl
Copy link
Contributor

@stancl stancl commented Mar 30, 2025

Addresses the regression introduced in #54811, basically optional parameter support. The feature slipped my mind when working on that PR, sorry about that. On the bright side the test suite should now cover pretty much everything.

I don't super love the way the offset logic is implemented. The idea of using an offset instead of conditionally reversing the parameters array like in my previous PR is probably solid, though this specific implementation is a bit... logic-dense.

I'd like this to be refactored at some point, which should be pretty risk-free given the (now even bigger) volume of tests, but since I want to fix the pretty serious regression fast this implementation should be reasonable enough.

Fixes #55209

stancl and others added 3 commits March 30, 2025 16:44
)

This reworks the logic from conditionally reversing passed parameters to
instead computing an offset and filling parameters left-to-right from there.
@taylorotwell taylorotwell merged commit 2730140 into laravel:12.x Mar 30, 2025
39 checks passed
@taylorotwell
Copy link
Member

Yeah I wonder if we can simplify this a bit. Generating URLs is now probably the most complicated method in the framework most likely.

@stancl
Copy link
Contributor Author

stancl commented Mar 30, 2025

Yeah, I'm not happy with how the code ended up but at the same time think the general approach (centralizing all of the logic there with some key ideas for the "algorithm") makes the most sense.

Surprised the optional parameters weren't caught by tests, though that's now fixed, so as mentioned any refactors should be risk-free now. Honestly it's just a deceptively complex area since you have:

  1. Default values
  2. Parameters that use the default route key or some route-specific slug
  3. Optional parameters
  4. Even support for default parameters after required parameters
  5. Passing parameters with keys or positionally, or both

So there has to be some logic for handling what a human would expect here. Perhaps dropping 4) could simplify the code and tests a bit since I had to work around that and it isn't really a documented feature, more so an observation that the framework does support it. But also in relative terms it'd be just a minor simplification for a potentially significant breaking change — no idea how many people may be using that.

@duncanmcclean
Copy link
Contributor

Thanks for the quick fix, @stancl!

And for the quick tag @taylorotwell! 🙂

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnamed route parameters interpreted in reverse order
3 participants