Skip to content

[HttpKernel] Fix static code analysis warning #19960

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.

Sign up for GitHub

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

[HttpKernel] Fix static code analysis warning #19960

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
caaad2c
test container type
ro0NL Sep 18, 2016
6bff28c
consistent exception
ro0NL Sep 18, 2016
46d6e4b
@throws after @return
ro0NL Sep 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Symfony/Component/HttpKernel/Kernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,9 +673,15 @@ protected function dumpContainer(ConfigCache $cache, ContainerBuilder $container
* @param ContainerInterface $container The service container
*
* @return DelegatingLoader The loader
*
* @throws \InvalidArgumentException When the container is not an instance of ContainerBuilder
*/
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)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

- 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)));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Targets 2.7 / PHP 5.3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ro0NL ro0NL Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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").

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. issuing a deprecation notice in the current version
  2. allow people to fix their in the same version where the notice appeared
  3. then make the change. If the notice can't

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@nicolas-grekas nicolas-grekas Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😕

}

$locator = new FileLocator($this);
$resolver = new LoaderResolver(array(
new XmlFileLoader($container, $locator),
Expand Down