Fix path support after unlimited optional placeholders #292
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewnicols
force-pushed
the
optionalNestedGlobWithFollowingFolder
branch
from
d291392 to
37f2f19
Compare
|
Note: 1.3.0 is also impacted by this bug. |
andrewnicols
added a commit
to andrewnicols/moodle
that referenced
this pull request
Dec 10, 2024
See the following upstream issues for further information: - nikic/FastRoute#292 - nikic/FastRoute#293
andrewnicols
force-pushed
the
optionalNestedGlobWithFollowingFolder
branch
from
37f2f19 to
15eceef
Compare
|
Any chance of a review on this issue? Thanks |
When using a route which contains both an unlimited optional
placeholder, and another optional placeholder afterwards, the incorrect
values are collected.
For example, given the following route:
```
/go/to/[{location:.*}[/info/{subpage}]]
```
The following behaviour is currently observed:
- route: `/go/to/[{location:.*}[/info/{subpage}]]`
- url: `/go/to/australia/perth/info/about`
- location: `'australia/perth/info/about'`
- subpage: `''`
Note that the `location` contains `/info/about` and the `subpage` is
empty.
This is inconsistent with the behaviour where an unlimited value is
_not_ used:
- route: `/go/to/[{location}[/info/{subpage}]]`
- url: `/go/to/australia/info/about`
- location: `'australia'`
- subpage: `'about'`
In the case of the unlimited optional path, the expected behaviour is:
The correct value would be:
- route: `/go/to/[{location:.*}[/info/{subpage}]]`
- url: `/go/to/australia/perth/info/about`
- location: `'australia/perth'`
- subpage: `'about'`
This commit change updates the route dispatcher to reverse the order of
the routes when adding routes to the router, which brings the unlimited
path placeholder format inline with limited path placeholders.
Signed-off-by: Andrew Nicols <andrew@nicols.co.uk>
andrewnicols
force-pushed
the
optionalNestedGlobWithFollowingFolder
branch
from
15eceef to
33637cd
Compare
|
I believe I've solved all of the GHA failures. |
|
@andrewnicols I'll have a look soon. The problem here is that this will affect the performance of the matching process. Also it isn't really a bug, as unbound parameters aren't officially supported - thus the lack of tests for it. |
|
I don't think this needs a fix. The regex should be simply ungreedy, i.e. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using a route which contains both an unlimited optional placeholder, and another optional placeholder afterwards, the incorrect values are collected.
For example, given the following route:
The following behaviour is currently observed:
/go/to/[{location:.*}[/info/{subpage}]]/go/to/australia/perth/info/about'australia/perth/info/about'''Note that the
locationcontains/info/aboutand thesubpageis empty.This is inconsistent with the behaviour where an unlimited value is not used:
/go/to/[{location}[/info/{subpage}]]/go/to/australia/info/about'australia''about'In the case of the unlimited optional path, the expected behaviour is: The correct value would be:
/go/to/[{location:.*}[/info/{subpage}]]/go/to/australia/perth/info/about'australia/perth''about'This commit change updates the route dispatcher to reverse the order of the routes when adding routes to the router, which brings the unlimited path placeholder format inline with limited path placeholders.