laminas / laminas-diactoros

PSR HTTP Message implementations
https://docs.laminas.dev/laminas-diactoros/
BSD 3-Clause "New" or "Revised" License
487 stars 63 forks source link

Could ServerRequestFactory::marshallUriFromSapi() be made public? #122

Closed markstory closed 2 years ago

markstory commented 2 years ago

Feature Request

Q A
New Feature yes
RFC yes
BC Break yes

Summary

:wave: from your friends at the CakePHP project, we've been longtime users of diactoros :bow: We recently learned of the plans to deprecate and remove marshalUriFromSapi and eventually remove it.

https://github.com/laminas/laminas-diactoros/blob/e5b6419fea007b8b4a71034edc8ec96c88d4c57d/src/functions/marshal_uri_from_sapi.php#L30-L36

The other implementation (ServerRequestFactory::marshallUriFromSapi()) is currently private.

https://github.com/laminas/laminas-diactoros/blob/e5b6419fea007b8b4a71034edc8ec96c88d4c57d/src/ServerRequestFactory.php#L124

Is there any appetite in making this method public so that we could use it in our RequestFactory?

Ocramius commented 2 years ago

Depends on the use-case: it would need to be extracted from ServerRequestFactory, IMO.

Wouldn't using this work?

https://github.com/laminas/laminas-diactoros/blob/0b7d4e8ff6560e3a7f84d16283f2e12402f2bd78/src/ServerRequestFactory.php#L67

Can you build a request and then get the URI from there?

ADmad commented 2 years ago

Can you build a request and then get the URI from there?

We build our own ServerRequest instance (using the various global functions Diactoros provides), so building Diactoros' ServerRequest instance just to get the Uri instance would be quite wasteful. All the parsing of PHP's global environment/request variables is non trivial amount of code, so would like to avoid doing it twice.

Ocramius commented 2 years ago

Well aware, it's just that the public API becomes much bigger there.

If somebody can send me a patch to try and extract marshalUriFromSapi() into a well documented public API on a new class that we test independently (and can deprecate separately, if needed in future), that would probably be the best way forward :)

Ocramius commented 2 years ago

Handled in #123