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 optional parameters in url generator #2246

Merged
merged 3 commits into from
Jul 29, 2020

Conversation

askvortsov1
Copy link
Sponsor Member

Fixes #1972

Changes proposed in this pull request:

  • Select the most fitting paths option instead of the first one to support using url generator with optional parameters
  • Add unit tests for this functionality

Reviewers should focus on:

  • Any shortcomings in the algorithm I used?
  • Any code style recommendations? The variable naming I feel a bit iffy on

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • [x[ Backend changes: tests are green (run composer test).

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

I'm ok with the approach and the unit tests seem fine. I do find the actual implementation quite difficult to read.

Could you try to make clear which variables reference an index and which variables reference a count?

I'm not actually sure what our preference is between snake_case and camelCase but I was under the impression we mostly used camelCase?

src/Http/RouteCollection.php Outdated Show resolved Hide resolved
src/Http/RouteCollection.php Outdated Show resolved Hide resolved
@tankerkiller125
Copy link
Contributor

I'll wait to review until the things clark pointed out are fixed. Like clark I also have difficulty reading the actual implementation so some clarification of variables would be great.

@askvortsov1
Copy link
Sponsor Member Author

Oh boy this code is a mess, not sure what I was thinking when I wrote it... I did a bit of cleanup, that was embarrasing...

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

That looks much better.

I still find the use of the index as the "number" of parameters a bit confusing. This only works because we expect all $parts to be a repetition of the same elements with further entries just having more elements after the common elements.

This might not be obvious to code readers unfamiliar with Fastroute.

If all $parts are always sorted in ascending number of parts, maybe this could even be simplified to break as soon as one result doesn't have as many matches as the previous entry. Not sure if the optimization is worth it though as most routes will probably only ever have two routes variants at most.

@askvortsov1 askvortsov1 merged commit 8a73cc5 into master Jul 29, 2020
@askvortsov1 askvortsov1 deleted the as/fix_optional_parameters_in_url_generator branch July 29, 2020 00:51
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.

Optional route parameters cannot be filled via the url builder
4 participants