search

FriendsOfSymfony / FOSJsRoutingBundle

A pretty nice way to expose your Symfony routing to client applications.
1.48k stars 261 forks source link

All absolute url generation should include ports #361

Closed rakelley closed 4 years ago

rakelley commented 4 years ago

The port support added in #320 currently only works when the route has a host token that differs from the provided context. In reality, if we need a port, we need it whenever an absolute URL is generated.

Note: I don't care for the repeated ternary concatenation and would change it, but @emulienfou made specific emphasis on this in the original PR comments, so I preserved it.

tobias-93 commented 4 years ago

Thanks @rakelley

wadjeroudi commented 4 years ago

@tobias-93 I think this PR should be reverted because of #379. The test is not correct, the host already includes non standard ports so in the test the host should be localhost:1234 https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/blob/master/Extractor/ExposedRoutesExtractor.php#L137 So the port was already present with non standard ports, and if you need to include it for standard ports, you should tweak the getPort function. https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/blob/master/Extractor/ExposedRoutesExtractor.php#L146

dew326 commented 4 years ago

Hi @tobias-93, I created PR for the issue with non standards ports (https://github.com/FriendsOfSymfony/FOSJsRoutingBundle/pull/381). Could you take a look?