Add RouteHelper, wrapping Laminas URL helper#5049
Conversation
module/VuFind/tests/unit-tests/src/VuFindTest/Http/RouteHelperTest.php
Outdated
Show resolved
Hide resolved
|
GHA is down again so I'm not sure it builds, but I think it's worth a review anyway given the earlier discussion. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @maccabeelevine, this is off to a good start. See below for suggestions!
module/VuFind/tests/unit-tests/src/VuFindTest/Http/RouteHelperTest.php
Outdated
Show resolved
Hide resolved
module/VuFind/tests/unit-tests/src/VuFindTest/Http/RouteHelperTest.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
demiankatz
left a comment
There was a problem hiding this comment.
@maccabeelevine, is there anything else pending on your end before we merge this?
As noted elsewhere, we may need to figure out how to handle the normalize_path option, which we have to switch off in some situations to prevent problems. However, I'm not sure that we need to expose that as an option. It may actually be better to just always turn the option off, unless we can find a scenario where leaving it on is important. (The reason we need to disable it in some scenarios is to prevent IDs with encoded slashes from getting decoded and causing problems; I'm not aware of any scenarios where we actually need it enabled -- though perhaps switching it off and running tests will reveal something I'm unaware of). I'm open to the idea of merging this as-is and starting to refactor so we can figure that out later -- worst case scenario, we add an optional $options (or more specific) parameter on the end of the signature here without changing the other work.
What do you think? I'm also interested in @EreMaijala's thoughts.
|
This looks good to me, but I think we should disable the normalize_path option here at least by default. |
demiankatz
left a comment
There was a problem hiding this comment.
I agree with @EreMaijala's suggestion. What do you think of the below?
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
|
Nothing else pending on my end. |
demiankatz
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks, @maccabeelevine!
For #4939, discussed here: https://github.com/vufind-org/vufind/pull/4939/changes#r2736633817
Wrap Laminas URL helper so it can be used outside of the view rendering context, specifically as part of an API implementation. Similar to #4983.