Conversation
michalsn
left a comment
There was a problem hiding this comment.
I understand the motivation, especially since the hostname name may suggest that the port should not be considered.
However, I'm not sure we need to change the existing behavior. Currently, the option is compared against HTTP_HOST, which allows users to include the port when necessary - for example, localhost:8080. This also preserves the ability to distinguish the same hostname on different ports.
Would you be open to changing this into a documentation-only PR? We could clarify that the match is against the request host value and that, when it contains a non-standard port, the port must also be included in the route option.
|
Thanks for the feedback! I understand the concern about backward compatibility and port-based routing. However, requiring users to hardcode ports in routes (e.g., localhost:8080) is problematic across different environments (local Docker/spark serve vs production). How about a backward-compatible compromise? If the route's hostname option explicitly includes a port (e.g., 'localhost:8080'), we match it against the full host:port (preserving current behavior). If the route's hostname does not include a port (e.g., 'example.com' or 'localhost'), we strip the port from the incoming request before comparison. This avoids any breaking changes for existing code while making standard hostname routing work seamlessly across environments with non-standard ports. If you still prefer not to modify the behavior in code, I can update this PR to be documentation-only as suggested. |
|
The problem itself is real. Anyone running on Docker or That said, I don't think stripping the port in the router is the right fix. Today a route restricted to I would prefer we leave the matching as is and document clearly that hostname restrictions compare against the full |
The hostname and subdomain options are compared against the full HTTP_HOST value, including the port. Document that routes restricted by these options must include the port when the application is served on a non-standard port (e.g. localhost:8080, Docker, spark server).
91ceff9 to
598f2f7
Compare
|
@michalsn thanks for the detailed feedback — you make a fair point about the existing example.com:8080 behavior and about where host parsing belongs. I've updated the PR accordingly: it is now documentation-only. It clarifies in the routing guide that the hostname (and subdomain) options are compared against the full HTTP_HOST value, including the port, so routes restricted by them must include the port when the app is served on a non-standard port (e.g. localhost:8080, Docker, spark server). That said, I still think the underlying problem deserves a proper fix at some point, and I'd like your take on this before pursuing it separately: SiteURIFactory::getHost() already strips the port from HTTP_HOST, and the Restrict attribute matches via getUri()->getHost() (no port) — so hostname restrictions behave differently depending on whether you use the route option or the attribute. A follow-up fix in the URI/request layer (so RouteCollection uses the same port-stripped host as Restrict) would make the behavior consistent and would mean ['hostname' => 'example.com'] just works on any port. Would you be open to that as a separate PR, or is the current behavior intentional on the route side? |
Description
Documentation-only PR. The
hostname(andsubdomain) route options are comparedagainst the full
HTTP_HOSTvalue of the request, including the port when one ispresent. This is now clarified in the routing guide so users serving the app on a
non-standard port (e.g.
localhost:8080, Docker,spark server) know they mustinclude the port in the route option for the routes to match.
Checklist: