[HttpKernel] Fix static code analysis warning #19960

ro0NL · 2016-09-18T10:29:10Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#19867
License	MIT
Doc PR	reference to the documentation PR, if any

Before:

[Symfony\Component\Debug\Exception\ContextErrorException]                                                                                                                                                                                                                                                                                                
  Catchable Fatal Error: Argument 1 passed to Symfony\Component\DependencyInjection\Loader\FileLoader::__construct() must be an instance of Symfony\Component\DependencyInjection\ContainerBuilder, instance of Symfony\Component\DependencyInjection\Container given

After

[InvalidArgumentException]                                                                                                                      
  The container must be an instance of Symfony\Component\DependencyInjection\ContainerBuilder, got Symfony\Component\DependencyInjection\Container.

Cosmetic only (better error though), but fixes static code analysis warnings.

Ideally in 4.0 the signature is updated...

chalasr · 2016-09-18T10:53:33Z

src/Symfony/Component/HttpKernel/Kernel.php

     * @return DelegatingLoader The loader
     */
    protected function getContainerLoader(ContainerInterface $container)
    {
+        if (!$container instanceof ContainerBuilder) {
+            throw new \DomainException(sprintf('Container must be an instance of Symfony\Component\DependencyInjection\ContainerBuilder, got %s', get_class($container)));


Just my opinion but I would prefer see The first argument of ...\Kernel::getContainerLoader() must be an instance of... for faster understanding

Not sure.. i tend to not couple context and domain. The message is about the container state, not whether it's the first, second or 10th argument.

It can be a DX improvement.. but to be honest i think when calling getContainerLoader($myContainerVariableName) and it throws Container must be ... is pretty straightforward ;-)

Got your point, both are arguable I think.
Edit: Got it if your message was $container must be :) (It's how Guard does it for instance)

Im lost on symfony and exceptions. Based on this ref i think we should do that.

I tend to agree the current message is clear enough. The stacktrace is here in case we need more clues.

ogizanagi · 2016-09-18T18:14:13Z

src/Symfony/Component/HttpKernel/Kernel.php

     * @return DelegatingLoader The loader
     */
    protected function getContainerLoader(ContainerInterface $container)
    {
+        if (!$container instanceof ContainerBuilder) {
+            throw new \InvalidArgumentException(sprintf('The container must be an instance of Symfony\Component\DependencyInjection\ContainerBuilder, got %s.', get_class($container)));


- throw new \InvalidArgumentException(sprintf('The container must be an instance of Symfony\Component\DependencyInjection\ContainerBuilder, got %s.', get_class($container))); + throw new \InvalidArgumentException(sprintf('The container must be an instance of %s, got %s.', ContainerBuilder::class, get_class($container)));

Targets 2.7 / PHP 5.3.

Yeah. Sorry. I saw the target branch after my comment but lose my connection, so was unable to remove my comment :)

The only option would be to change the type hint of the method, but given that this is a protected method, this is not possible and very unlikely to happen, so it looks like a won't fix to me.

Eh.. i'd say this change + 4.0 changelog about changing signature. Otherwise this change only (ie. not sure why this would be "wont fix").

It would be "won't" fix even in 4.0 because we have a policy of:

issuing a deprecation notice in the current version

allow people to fix their in the same version where the notice appeared

then make the change. If the notice can't

Since 2. is not possible here, neither is 3. This ensures a continuous upgrade path.

Not sure i follow.. this is only cosmetic. It crashed before as well... what should we deprecate?

I was talking about changing the method signature to ContainerBuilder instead of ContainerInterface, which is not possible, because there is no continuous upgrade path (except creating a new method or deprecate extending this one, but is it worth it?)

It's also about performance and not checking types ourselves. It crashed before, it will throw an exception, and anyway, use cases where you really pass a non-ContainerBuilder are pretty small. So, I really think this is not worth it. At the minimum, why not just change the phpdoc?

Fair enough, i tend to look at it more abstract. Ie. the current design is semi-flawed, and could be fixed imo. I dont like to depend on PHP what will happen..

why not just change the phpdoc

Not sure.. imo. it's more confusing.

Lets keep it as is then 😕

chalasr · 2016-09-19T10:49:51Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -672,10 +672,16 @@ protected function dumpContainer(ConfigCache $cache, ContainerBuilder $container
     *
     * @param ContainerInterface $container The service container
     *
+     * @throws \InvalidArgumentException When the container is not an instance of ContainerBuilder


The @throws annotation should go after the @return one (ref #18975)

carsonbot added Status: Needs Review Bug labels Sep 18, 2016

ro0NL changed the title ~~test container type~~ [HttpKernel] Fix static code analysis warning Sep 18, 2016

test container type

caaad2c

chalasr reviewed Sep 18, 2016

View reviewed changes

consistent exception

6bff28c

ogizanagi approved these changes Sep 18, 2016

View reviewed changes

chalasr reviewed Sep 19, 2016

View reviewed changes

@throws after @return

46d6e4b

ro0NL closed this Oct 3, 2016

ro0NL deleted the http-kernel/container-loader branch December 17, 2016 07:15

ro0NL mentioned this pull request Dec 17, 2016

[Security] Fix error when calling HttpUtils::generateUri() #20946

Closed

ro0NL mentioned this pull request Jul 16, 2017

[Routing][Translation] Assert return value for PHP configuration #23522

Closed

javiereguiluz mentioned this pull request Oct 18, 2017

Data types are not strict #19867

Closed

ro0NL mentioned this pull request Nov 23, 2017

[Security][Factory] use ArrayNodeDefinition instead of NodeDefinition #25112

Closed

Uh oh!

[HttpKernel] Fix static code analysis warning #19960

[HttpKernel] Fix static code analysis warning #19960

Uh oh!

Conversation

ro0NL commented Sep 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Sep 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ro0NL commented Sep 18, 2016 •

edited

Loading

chalasr Sep 18, 2016 •

edited

Loading

ro0NL Sep 19, 2016 •

edited

Loading

nicolas-grekas Oct 3, 2016 •

edited

Loading

chalasr Sep 19, 2016 •

edited

Loading