Open Julian-Louis opened 2 weeks ago
codecov-commenter
commented
2 weeks ago Attention: Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.
Project coverage is 8.32%. Comparing base (
f0b3801) to head (93ba9e8). Report is 2 commits behind head on master.
bwoebi
commented
2 weeks ago Hey @Julian-Louis,
thanks for taking the initiative here! I think this approach may not be fully correct.
Like, normalization is basically trying to recognize ids, but if you have routes which contain a name / a string identifier, not matching these patterns, the parameter will be assigned wrongly, I think.
E.g. let's assume a route /posts/{category}/{id} - we do a request /posts/cooking/12.
We'll end up with a normalized form /posts/cooking/?.
The parameters being replaced, it will be /posts/cooking/{category}, which is not right.
Julian-Louis
commented
2 weeks ago Hello, thank you for your feedback ! I understand your concerns and will explore alternative implementations. I'll update the code as soon as I can
Julian-Louis
commented
2 weeks ago Hello @bwoebi,
I revisited my code and implemented a new approach! By using a reflection class, I had access to the Symfony container, allowing me to utilize the router matcher. This allows me to accurately retrieve the actual parameters and URL of the route. And I normalized the URL by replacing parameter keys with corresponding values in the URL.
What do you think of it ?
bwoebi
commented
2 weeks ago Hmm @Julian-Louis, I think this code has another missing case:
Now a route /posts/{category}/{id} matching /posts/1/1 will be converted to /posts/{category}/{category}, if I read that code correctly?
I've been looking through the Symfony code right now, and am finding Router::getRouteCollection(), which contains all routes with their names and paths. Either the $router->options['cache_dir'] is set and the Router::getRouteCollection() is loaded if uncached or it's not set and it's always present. So, isn't it simply possible to fill our own cache file with our own name=>paths mapping if Router::getRouteCollection() is called and use this (and calling it once if cache doesn't exist yet)? (if cache is enabled, otherwise, we'll just walk the routecollection directly?)
A direct mapping from a cache file should be quite fast then I suppose?
Note that I have no idea what I'm talking about though, not an expert in Symfony, just navigating through their codebase and trying to figure out what's going on there. Please tell me if my suggestion is unreasonable :-)
Julian-Louis
commented
2 weeks ago @bwoebi
I just updated my code and now /posts/{category}/{id} (/posts/1/1) will be converted to /posts/{category}/{id} instead of /posts/{category}/{category} anymore, sorry for the mistake.
I'm not a big symfony expert too, but I'll try to take a look on the way you suggest
bwoebi
commented
2 weeks ago I think the current solution is probably viable. It's not 100% correct (e.g. /posts/posts with a route /posts/{name} will be now /{name}/posts).
I'm not sure if that's the only edge case, it's maybe edge case enough to be accepted.
EDIT: The regex also matches too much, needs to bound check for a preceding / and $|/ at the end.
I also have no idea of the performance characteristics; I'll leave that to somebody else from my team to evaluate :-)
Hello, this is draft pull request adds the http.route tag to the Symfony integration. Noticing that PR #2477 was closed, I decided to take the initiative to implement this feature.
Can you please provide your feedback on this proposal/implementation? If approved, I'll proceed with implementing tests to further validate the changes.
Thank you for your review !
Reviewer checklist