Skip to content

[4.0][BC Break] Removed BC layers for ControllerResolver::getArguments() #22779

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

Closed
wants to merge 3 commits into from
Closed

[4.0][BC Break] Removed BC layers for ControllerResolver::getArguments() #22779

wants to merge 3 commits into from

Conversation

linaori
Copy link
Contributor

@linaori linaori commented May 19, 2017

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

Removes the Backwards Compatibility layer for the ControllerResolver that depends on the ArgumentValueResolver. There's still 1 bit left in the HttpKernel, but I don't quite know how this is solved in the best way:

    public function __construct(EventDispatcherInterface $dispatcher, ControllerResolverInterface $resolver, RequestStack $requestStack = null, ArgumentResolverInterface $argumentResolver = null)
    {
        $this->dispatcher = $dispatcher;
        $this->resolver = $resolver;
        $this->requestStack = $requestStack ?: new RequestStack();
        $this->argumentResolver = $argumentResolver;

        if (null === $this->argumentResolver) {
            @trigger_error(sprintf('As of 3.1 an %s is used to resolve arguments. In 4.0 the $argumentResolver becomes the %s if no other is provided instead of using the $resolver argument.', ArgumentResolverInterface::class, ArgumentResolver::class), E_USER_DEPRECATED);
            // fallback in case of deprecations
            $this->argumentResolver = $resolver;
        }
    }

The 4th argument is now mandatory, but I can't make it mandatory without switching it with the request stack.

  • I can make both mandatory
  • I can make it ?RequestStack
  • I can switch the arguments

Each of those area a BC break but for the request stack or the switch, there is no BC layer yet (could be done in 3.4).

@stof
Copy link
Member

stof commented May 19, 2017

The argument is not meant to become mandatory, but to instantiate an ArgumentResolver if none is provided, according to the deprecation message

@linaori
Copy link
Contributor Author

linaori commented May 19, 2017

@stof you're right.

@linaori
Copy link
Contributor Author

linaori commented May 19, 2017

Minor addition, @nicolas-grekas has added the service resolver, but this one isn't added to the default set.

@stof
Copy link
Member

stof commented May 19, 2017

@iltar that's expected, as this ServiceResolver requires some wiring, it is not standalone. So using it requires configuring the resolver externally and passing it to the kernel (which is what FrameworkBundle does for instance)

@linaori
Copy link
Contributor Author

linaori commented May 19, 2017

@stof fair enough, I see that the dependency is optional as well. I got confused because it's between the other resolvers here: https://github.com/symfony/symfony/tree/master/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver

@nicolas-grekas
Copy link
Member

fabbot failure looks legit

@linaori
Copy link
Contributor Author

linaori commented May 20, 2017

@nicolas-grekas indeed, should be fixed now!

@nicolas-grekas
Copy link
Member

Thank you @iltar.

@fabpot fabpot mentioned this pull request Oct 19, 2017
@linaori linaori deleted the cleanup/argument-value-resolver branch February 8, 2019 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants