Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Routing] UrlHelper to get absolute URL for a path #30862

Merged
merged 1 commit into from Apr 7, 2019

Conversation

@vudaltsov
Copy link
Contributor

commented Apr 4, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR todo

I noticed that I need to generate absolute urls quite often. For example when normalizing uploads in API. I found Twig's absolute_url() really helpful, but obviously Symfony\Bridge\Twig\Extension\HttpFoundationExtension cannot be used as a normalizer's argument service.

In this PR I propose to extract HttpFoundationExtension::generateAbsoluteUrl and HttpFoundationExtension::generateRelativePath to separate interfaces which could be used on their own. Although this could be just a final class helper, I thought that we might leave a possibility for decoration here. That's why I created interfaces.

  • Split HttpFoundationExtension into two interfaces
  • Deprecate HttpFoundationExtension::generateAbsoluteUrl and HttpFoundationExtension::generateRelativePath
  • Add service definitions
  • Fix tests
  • Add docs
@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@vudaltsov could you please show a small before/after example ... and maybe also briefly list the changes that existing apps may need to do. Thanks.

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@javiereguiluz , sure.

final class UserApiNormalizer implements NormalizerInterface
{
    private $absoluteUrlGenerator;

    public function __construct(AbsoluteUrlGeneratorInterface $absoluteUrlGenerator)
    {
        $this->absoluteUrlGenerator = $absoluteUrlGenerator;
    }

    public function normalize($user, $format = null, array $context = [])
    {
        return [
            // ...
            'avatar' => $this->absoluteUrlGenerator->generateAbsoluteUrl($user->avatar()->path()),
            // ...
        ];
    }

    // ...
}

No changes needed for existing apps. Everything will continue to work as is. But two useful helping services will appear.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

AbsoluteUrlGeneratorInterface looks like a routing concern.. probably caused by the "generate" term. Im a bit skeptical about introducing these interfaces under these names. Do we want/need the contracts even?

Maybe RequestContext::toAbsolute/RelativeUrl()?

@stof

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

naming them UrlGenerator will indeed create confusion with the routing component

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Agree, I will consider different naming today.

@ro0NL , RequestContext is not aware of the RequestStack, so it's not gonna work. This helper uses them both to get maximum information to generate an absolute url.

As I explained, I created contracts because you might need to decorate these services once you have some additional context for generating an absolute url.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

My idea was RequestContext::toAbsoluteUrl(Request $request = null). From a controller pov you have the request at hand.

decorate these services once you have some additional context for generating an absolute url

is this a feature we want to sell? URL composition is a fixed concept IMHO.

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@ro0NL , I understand the idea of passing Request as an argument. We'll be able to generate paths for any request then.

The problem is that DX is worse since almost always I will have to inject two services instead of one (+RequestStack) into non request aware services. Also I don't think it is a responsibility of the RequestContext to do this — it's just keeping and providing request data. And I also doubt that I will ever need this in controllers. Mostly in other services, used by commands and message handlers as well (see normalizer example above).

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Refactored to UrlHelper without interfaces. Looks better? I don't really like the word helper, honestly...

@linaori

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Why is it final without an interface? Mocking this in userland will be a pain due to its dependencies and internal behavior. If it's something developers shouldn't use, I'd expect it to be @internal.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

one could pass a request stack with mocked requests, and any requestcontext. Im not sure utilities need interfaces.

@linaori

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

That creates a massive overhead of objects that you have to bring into the correct state in order to test your function, while their internal behavior is of no concern to your particular test if mocking would be sufficient.

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

what about putting the interface in symfony/contracts? It's a bit weird for HttpFoundation (a specific concept) to introduce a polymorphic matter.

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I would not create new abstraction for that. Best would be to add that to the Request class instead.

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@fabpot, this cannot be done in Request because we need RequestContext as a fallback. The idea of these helper utilities is to provide absolute urls even outside the request scope (commands, for example).

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 5, 2019

@vudaltsov vudaltsov force-pushed the vudaltsov:absolute-url-generator branch from af2971f to bb346b6 Apr 6, 2019

@vudaltsov vudaltsov changed the title [HttpFoundation] Add AbsoluteUrlGeneratorInterface and RelativePathGeneratorInterface [Routing] UrlHelper to get absolute URL for a path Apr 6, 2019

@vudaltsov vudaltsov marked this pull request as ready for review Apr 6, 2019

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

After discussing with @fabpot , moved to Routing, since the helper depends on the RequestContext which is a routing class.

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

Btw, what's the best way to handle deprecations in HttpFoundationExtension?
Add @expectedDeprecation?

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

@fabpot , looked for places in Symfony where I could replace some code with a helper call (except the Twig HttpFoundationExtension). Two looked relevant:

  1. if (null !== $this->secureDomainRegexp && 'https' === $this->urlMatcher->getContext()->getScheme() && preg_match('#^https?:[/\\\\]{2,}+[^/]++#i', $path, $host) && !preg_match(sprintf($this->secureDomainRegexp, preg_quote($request->getHttpHost())), $host[0])) {
    .

But it seems that UrlHelper will not really help there :)

@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

*
* @author Valentin Udaltsov <udaltsov.valentin@gmail.com>
*/
final class UrlHelper

This comment has been minimized.

Copy link
@greg0ire

greg0ire Apr 6, 2019

Contributor

Alternative names: UrlManipulator, UrlBuilder

This comment has been minimized.

Copy link
@vudaltsov

vudaltsov Apr 6, 2019

Author Contributor

UrlManipulator is nice. Let's see what other reviewers think.
UrlBuilder is bad as a stateless service name.

@xabbuh
Copy link
Member

left a comment

we need to document the deprecations in the UPDATE-4.3.md and UPGRADE-5.0 files

@vudaltsov vudaltsov force-pushed the vudaltsov:absolute-url-generator branch 2 times, most recently from df6d96c to 3f7c70f Apr 6, 2019

Show resolved Hide resolved UPGRADE-4.3.md Outdated
Show resolved Hide resolved UPGRADE-5.0.md Outdated
Show resolved Hide resolved src/Symfony/Bridge/Twig/CHANGELOG.md Outdated
@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

@xabbuh , done!

@xabbuh

xabbuh approved these changes Apr 7, 2019

@vudaltsov vudaltsov force-pushed the vudaltsov:absolute-url-generator branch from 203dce8 to 744c668 Apr 7, 2019

@fabpot

fabpot approved these changes Apr 7, 2019

Copy link
Member

left a comment

minor changes to be done before merge

Show resolved Hide resolved src/Symfony/Bridge/Twig/Extension/HttpFoundationExtension.php Outdated
Show resolved Hide resolved UPGRADE-4.3.md Outdated
@vudaltsov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

Ready!

@fabpot fabpot force-pushed the vudaltsov:absolute-url-generator branch from 0ff8219 to 388d8f5 Apr 7, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

Thank you @vudaltsov.

@fabpot fabpot merged commit 388d8f5 into symfony:master Apr 7, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 7, 2019

feature #30862 [Routing] UrlHelper to get absolute URL for a path (vu…
…daltsov)

This PR was squashed before being merged into the 4.3-dev branch (closes #30862).

Discussion
----------

[Routing] UrlHelper to get absolute URL for a path

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | todo

I noticed that I need to generate absolute urls quite often. For example when normalizing uploads in API. I found Twig's `absolute_url()` really helpful, but obviously `Symfony\Bridge\Twig\Extension\HttpFoundationExtension` cannot be used as a normalizer's argument service.

In this PR I propose to extract `HttpFoundationExtension::generateAbsoluteUrl` and `HttpFoundationExtension::generateRelativePath` to separate interfaces which could be used on their own. Although this could be just a final class helper, I thought that we might leave a possibility for decoration here. That's why I created interfaces.

- [x] Split `HttpFoundationExtension` into two interfaces
- [x] Deprecate `HttpFoundationExtension::generateAbsoluteUrl` and `HttpFoundationExtension::generateRelativePath`
- [x] Add service definitions
- [x] Fix tests
- [ ] Add docs

Commits
-------

388d8f5 [Routing] UrlHelper to get absolute URL for a path
@OskarStark

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

Very nice feature Valentin 💥
Would you mind creating a docs PR please? 🙏

@vudaltsov vudaltsov deleted the vudaltsov:absolute-url-generator branch Apr 18, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.