Server URL Helper Cleanup #160
Conversation
|
Oh… GHA is down again… |
|
@gsteel I re-opened this: was it closed on purpose? |
|
@Ocramius - GHA confused me by being down and I had a duplicate pull - I left this one open and closed the other one. |
|
@gsteel if you can rebase this, I can throw it into |
- Deprecate all methods and properties in the server url helper - Introduce constructor to provide the helper with a configured url - Deprecate existing tests to make them easy to remove in future Signed-off-by: George Steel <[email protected]>
Adds laminas-diactoros as a dependency so that detection of the host URI can be delegated to existing code Signed-off-by: George Steel <[email protected]>
|
Nvm, needs some work, removing milestone for now, and releasing. |
Signed-off-by: George Steel <[email protected]>
gsteel
force-pushed
the
helpers-server-url-cleanup
branch
from
0106980 to
3bd7f84
Compare
…ng the deprecated method marshalUriFromSapi Signed-off-by: George Steel <[email protected]>
| @@ -30,6 +30,7 @@ | |||
| "ext-filter": "*", | |||
| "ext-json": "*", | |||
| "container-interop/container-interop": "^1.2", | |||
| "laminas/laminas-diactoros": "^2.13.0", | |||
There was a problem hiding this comment.
I don't think we should drag in a dependency for this 🤔
There was a problem hiding this comment.
I'm with you there, but I considered it better than attempting to parse $_SERVER inside this particular view helper.
Perhaps just abandon the patch and remove the helper instead - i.e. make sure that both MVC and Mezzio have their own implementations to replace it prior to a release of 3.0.
There are probably other helpers that could do with the same treatment - thinking url and perhaps baseUrl at the very least.
There was a problem hiding this comment.
Overall, I think this does bring value, and is implemented the right/safe way, but it also leads to the added dependency, which is a lot 🤔
Not sure what to do here. /cc @weierophinney maybe?
There was a problem hiding this comment.
I would likely copy over that functionality here and refactor to work with laminas-http. Since Diactoros is a PSR-7 library, there's a mismatch with the primary target of laminas-view (laminas-mvc). There's a reason we provide alternate url() and serverUrl() helpers in the mezzio-template-laminasview package...
There was a problem hiding this comment.
Can we/I create an entirely new library to contain everything that's MVC specific so that it can be stripped out in 3.0 - this could include all the supporting code around MVC events. If that's an agreeable path, I could just abandon this patch and deprecate the helper here entirely instead?
There was a problem hiding this comment.
I think that would be perfect, @gsteel!
There was a problem hiding this comment.
And we have to make sure that there is also a solution to create URLs with the same helpers when using laminas-cli. The server URL and the base path must be able to set via configuration, factory or something else.
There was a problem hiding this comment.
@froschdesign I'd argue that should be via separate helpers. Not quite sure how to manage it (right now, CLI config is the same as web config), but having separate helpers for separate contexts makes a lot of sense here to me.
There was a problem hiding this comment.
I've made a start on this with just the ServerUrl and Url helpers specifically for MVC:
If this is a good enough starting point, is transferring the repo ownership an option?
There was a problem hiding this comment.
Yes - we usually vote on any new packages in the TSC; this sort of thing can happen between meetings, though. Feel free to initiate; contact me in Slack for tips on how to do it.
Looking forward to further decoupling!
|
Closing… With laminas/laminas-mvc-view#1 in progress, I suspect we'll simply delete the Url and ServerUrl helpers here in 3.0 release. |
Description
Deprecates all methods in the server url helper and introduces a factory to provide the helper with the detected or configured host URI.
Brings in a hard dependency on laminas-diactoros in order to re-use the functions for detecting the host/port from the environment that are already present there.
Doesn't break BC according to tests, and also doesn't alter existing behaviour for long-running apps because the previous implementation caches the host/port in instance properties anyway.
The main improvement here is that consumers can configure the desired host and port without any attempt to figure it out from the environment which is particularly useful on the CLI or any other time when the required env vars are either wrong or not there at all.
The introduction of the factory is a potential BC break because an exception is thrown when there is neither configuration, nor usable env vars.