fix: support decimal numbers in route parameters#52
Open
thisismyurl wants to merge 3 commits into
Open
Conversation
Routes with parameters like :version now correctly match decimal numbers such as 1.5.1 and semantic version strings. Root cause: AltoRouter's default parameter pattern [a-zA-Z0-9_-] excludes dots. Solution: Define a custom 'slug' match type with pattern [a-zA-Z0-9._-]+ and use it for all default route parameters. Changes: - Add custom 'slug' match type to AltoRouter in ensure_router() - Modify convert_route() to use [slug:param] instead of [:param] - Add testRouteWithDecimalParameter() test case - Update docblock comments Backward compatible: Existing routes continue to work unchanged. Fixes Upstatement#45 (full disclosure: AI helped me identify the issue and verify my work)
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds support for route parameters that include dots (e.g., semantic version strings like 1.5.1) by introducing a custom AltoRouter match type and updating the route conversion logic, along with a regression test.
Changes:
- Add a custom AltoRouter match type (
slug) that allows dots in path parameters. - Change
convert_route()to convert:paramsegments into[slug:param]. - Add a PHPUnit test covering a decimal/semantic-version route parameter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| tests/RoutesTest.php | Adds a regression test ensuring routes can match dotted version parameters. |
| Routes.php | Adds a slug match type and updates route conversion to use it by default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+146
to
+147
| * Note: Default parameters are converted to [slug:param] to support dots, | ||
| * underscores, and hyphens in parameter values (e.g., version numbers like 1.5.1). |
Collaborator
|
Hi @thisismyurl , thank you for this contribution and taking on this long open issue. Copilot did some nitpicking, if you could take a look at those as well, it would be great! |
Levdbas
approved these changes
Jun 11, 2026
Collaborator
|
@jarednova , can you take a look at this as well/merge? |
This file contains hidden or 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
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.
Routes with parameters like :version now correctly match decimal numbers such as 1.5.1 and semantic version strings.
Previously, a route like
Routes::map('download/:version', callback);would work with/download/releasebut not/download/1.5.1.Root Cause
AltoRouter's default parameter pattern excludes dots.
Solution
Define custom 'slug' match type with
[a-zA-Z0-9._-]+for all default route parameters.Changes
ensure_router()convert_route()to use[slug:param]instead of[:param]testRouteWithDecimalParameter()test caseBackward compatible: Existing routes continue to work unchanged.
Changelog
Testing
testRouteWithDecimalParameter()test for/download/1.5.1Credits
Thanks to @thisismyurl (AI-assisted identification and testing)
Closes #45