Implement addMatchTypes method#50
Conversation
Added a method to allow custom match types for AltoRouter.
Added documentation for the addMatchTypes method in README.md.
There was a problem hiding this comment.
Pull request overview
This PR adds a public wrapper API to expose AltoRouter custom match-type registration through the Routes WordPress plugin, along with README documentation for using it.
Changes:
- Added
Routes::addMatchTypes()as a wrapper around AltoRouter’saddMatchTypes(). - Documented
addMatchTypesusage inREADME.mdwith an example.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| Routes.php | Introduces a new addMatchTypes wrapper method and related PHPDoc. |
| README.md | Adds a new documentation section describing how to register custom match types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static function addMatchTypes($match_types) | ||
| { | ||
| global $upstatement_routes; | ||
| if (isset($upstatement_routes->router)) { | ||
| $upstatement_routes->router->addMatchTypes($match_types); | ||
| } |
There was a problem hiding this comment.
addMatchTypes() currently does nothing if it’s called before the first Routes::map(), because $upstatement_routes->router is only instantiated inside map(). The README example calls addMatchTypes() before map(), so custom match types won’t be applied. Consider initializing the router here when it’s missing (ideally by extracting the router setup logic from map() into a shared helper) or persisting pending match types and applying them when the router is created.
| public static function addMatchTypes($match_types) | ||
| { |
There was a problem hiding this comment.
Method brace placement is inconsistent with the rest of this file (e.g., map() uses { on the same line as the function signature). For consistency/readability, match the existing style here.
| public static function addMatchTypes($match_types) | |
| { | |
| public static function addMatchTypes($match_types) { |
| public static function addMatchTypes($match_types) | ||
| { | ||
| global $upstatement_routes; | ||
| if (isset($upstatement_routes->router)) { | ||
| $upstatement_routes->router->addMatchTypes($match_types); | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add PHPUnit coverage for Routes::addMatchTypes() to ensure custom match types are applied as expected (especially for the documented call order of addMatchTypes() before map()). This will prevent regressions in router initialization / match-type registration behavior.
| Send an optional status code. Defaults to 200 for 'Success/OK' | ||
|
|
||
| ## addMatchTypes | ||
| This method makes it possbile to add custom matchtypes in Routes. |
There was a problem hiding this comment.
Typo in README: “possbile” should be “possible”.
| This method makes it possbile to add custom matchtypes in Routes. | |
| This method makes it possible to add custom matchtypes in Routes. |
| Routes::addMatchTypes([ | ||
| 'oldID' => '@[0-9]++', | ||
| ]); | ||
|
|
There was a problem hiding this comment.
The README example calls Routes::addMatchTypes() before Routes::map(), but the current implementation only forwards match types if the router has already been created. Either update the example to reflect the required call order, or (preferably) update addMatchTypes() to initialize/apply match types even when called before any routes are mapped.
|
Thanks for the PR @Levdbas ! Copilot found a few documentation nit-picks that are worth resolving. The biggest Q though is this finding ...
Is there a timing/initialization issue here? Thanks! |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…match types functionality
…-altorouter-addMatchTypes
…ter-addMatchTypes
…ch_current_request method
|
Hey @Levdbas, this looks good! I noticed one behavior change though to make sure we thought through the implications. It has to do with base path handling that might affect subdirectory installs (e.g. mysite.org/blog/). The old code in map() had this check: So if someone on a subdirectory install wrote Routes::map('blog/my-page', ...), it would detect the overlap and set the base path to /, avoiding double-prefixing. The new ensure_router() always sets the base path from the site URL regardless, so that same route would break — AltoRouter strips /blog/ and then tries to match the leftover against blog/my-page. Is the change intentional? The new behavior might actually more predictable (the old one was kinda fragile — base path depended on whatever the first map() call happened to be). But if anyone on a subdir install was including the subdirectory in their routes, this would be a breaking change. |
|
Hi @jarednova , you are right. And as you said, I think this makes the behavior more predicable. I have included an additional section in the readme to cover this potential breaking change and how to change it. |
|
Awesome! looking good. Another Q as we approach merging ... On master, Routes.php has the standard WordPress plugin header (Plugin Name, Version, etc.), but this PR removes it. My thought is that (like Timber) we are changing this to a Composer-only package. When I think WP Plugin I think something that's generally additive (SEO tools, etc.) vs. a dependency (a la Timber). I have to admit, I'm well out of the WP game at this point so I want to follow how things generally work across the community. Does all that square w your intention? Then all we gotta do is merge and do the version tagging in GitHub |
|
I think, as Routes originated in Timber, most people by now use this as a dependency managed by composer, since installing this as a plugin was never communicated as an installment option to begin with. And since Routes is probably mission critical to begin with in a website, you don't want somebody accidentally deactivating the plugin since that would directly lead to critical errors. So yea, a composer only package makes total sense for me. |
|
Thanks again for all yr work @Levdbas . I've tagged the current master as 0.10.0 and with your PR created a v1.0-beta ... https://github.com/Upstatement/routes/releases/tag/1.0.0-beta.1 I'll give things a couple weeks to see if we (or other users/contributors) report any issues before moving to official 1.0! |
Added a method to allow custom match types for AltoRouter.