Fix path support after unlimited optional placeholders #293
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
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
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
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.
This is the 1.3 version of #292. Since this is a bug it should really be part of a 1.3.x release.
This commit is based on the v1.3.0 tag. Ideally this should be included in a v1.3.1 release to support those using the library who are not in an immediate position to upgrade.
===
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.