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

Already on GitHub? Sign in to your account

[DX][DI] Add compiler pass which check extistence of service class name #11315

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
Contributor

l3l0 commented Jul 5, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? not sure - I hope that no
Deprecations? no
Tests pass? yes
Fixed tickets #11301
License MIT
Member

weaverryan commented Jul 5, 2014

Nice work!

So, now the question is, does this break BC? Is there a situation where class will not exist during the compiler process but exist later at run-time?

Contributor

pyrech commented Jul 6, 2014

Just my 2 cents about naming :) I would use 'class' rather than 'name' to be consistent with the "class" configuration of the service :
CheckServiceClassPass instead of CheckServiceNamePass
BadServiceClass instead of BadServiceName

Anyway, good job!

Contributor

l3l0 commented Jul 6, 2014

@pyrech good catch :) I changed names.

Contributor

Nek- commented Jul 6, 2014

IMO it's not a BC break, but in reality it is for those who like hacking Sf, maybe somebody that read a lot of issue about DI will be able to confirm.

👍 Anyway, nice work :) .

Contributor

Koc commented Jul 6, 2014

How this change affects performance? Looks like it will loads all of the classes.

If yes - imho better add console commands for checking.

Contributor

l3l0 commented Jul 7, 2014

@Koc all classes should be loaded when container is compiled (not on every request), so probably can affect stuff in dev/debug mode IHMO. For prod configuration it should be fine. But I have not done any performance checks, so I cannot answer the question how this can affect performance.

Contributor

Koc commented Jul 7, 2014

when container is compiled (not on every request)

I forgot about this, sorry. It occurs only when tracked resources was changed.

@stof stof commented on the diff Jul 7, 2014

...orkBundle/Tests/Controller/ControllerResolverTest.php
@@ -109,8 +109,8 @@ public function testGetControllerOnNonUndefinedFunction($controller, $exceptionN
public function getUndefinedControllers()
{
return array(
- array('foo', '\LogicException', 'Unable to parse the controller name "foo".'),
- array('foo::bar', '\InvalidArgumentException', 'Class "foo" does not exist.'),
+ array('myfoo', '\LogicException', 'Unable to parse the controller name "myfoo".'),
+ array('myfoo::bar', '\InvalidArgumentException', 'Class "myfoo" does not exist.'),

@stof stof commented on an outdated diff Jul 7, 2014

...ependencyInjection/Compiler/CheckServiceClassPass.php
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Exception\BadServiceClassException;
+
+/**
+ * Checks your service has valid class name (service exists)
@stof

stof Jul 7, 2014

Member

service exists looks weird in this description

@stof stof commented on the diff Jul 7, 2014

...ependencyInjection/Compiler/CheckServiceClassPass.php
+ {
+ foreach ($container->getDefinitions() as $id => $definition) {
+ if ($this->allowsToCheckClassExistenceFor($definition) && !class_exists($definition->getClass())) {
+ throw new BadServiceClassException($id, $definition->getClass());
+ }
+ }
+ }
+
+ private function allowsToCheckClassExistenceFor(Definition $definition)
+ {
+ return $definition->getClass() && !$this->isFactoryDefinition($definition) && !$definition->isSynthetic();
+ }
+
+ private function isFactoryDefinition(Definition $definition)
+ {
+ return $definition->getFactoryClass() || $definition->getFactoryService();
@stof

stof Jul 7, 2014

Member

in case of a definition using factoryClass, it should check that the factory class exists

@l3l0

l3l0 Jul 8, 2014

Contributor

Yeah right :) I will add that.

Member

stof commented Jul 8, 2014

I think this should run after the removing passes. It does not make sense to throw an exception for a service which gets removed

Contributor

l3l0 commented Jul 18, 2014

@stof I added checking class for factory_class. I moved compiler pass to part running after the removing passes - this somehow force me to resolve class name again using ParameterBag.

@l3l0 l3l0 changed the title from [DI] Add compiler pass which check extistence of service class name to [DX][DI] Add compiler pass which check extistence of service class name Jul 18, 2014

@stof stof commented on an outdated diff Jul 18, 2014

...Component/DependencyInjection/Compiler/PassConfig.php
@@ -53,7 +53,7 @@ public function __construct()
new ResolveInvalidReferencesPass(),
new AnalyzeServiceReferencesPass(true),
new CheckCircularReferencesPass(),
- new CheckReferenceValidityPass(),
+ new CheckReferenceValidityPass()
@stof

stof Jul 18, 2014

Member

Please don't remove the trailing comma in multiline arrays. Our coding standards require them

@stof stof commented on an outdated diff Jul 18, 2014

...Component/DependencyInjection/Compiler/PassConfig.php
@@ -66,7 +66,11 @@ public function __construct()
new AnalyzeServiceReferencesPass(),
new RemoveUnusedDefinitionsPass(),
)),
- new CheckExceptionOnInvalidReferenceBehaviorPass(),
+ new CheckExceptionOnInvalidReferenceBehaviorPass()
@stof

stof Jul 18, 2014

Member

same here

@stof stof commented on an outdated diff Jul 18, 2014

...Component/DependencyInjection/Compiler/PassConfig.php
@@ -66,7 +66,11 @@ public function __construct()
new AnalyzeServiceReferencesPass(),
new RemoveUnusedDefinitionsPass(),
)),
- new CheckExceptionOnInvalidReferenceBehaviorPass(),
+ new CheckExceptionOnInvalidReferenceBehaviorPass()
+ );
+
+ $this->afterRemovingPasses = array(
+ new CheckServiceClassPass()
@stof

stof Jul 18, 2014

Member

I would make it the last one of the removing passes instead (keeping the after removing passes as the extension point)

@stloyd stloyd commented on an outdated diff Jul 19, 2014

...dencyInjection/Exception/BadServiceClassException.php
@@ -0,0 +1,18 @@
+<?php
+
+namespace Symfony\Component\DependencyInjection\Exception;
@stloyd

stloyd Jul 19, 2014

Contributor

Missing license note.

Contributor

l3l0 commented Jul 19, 2014

@stof @stloyd done. I squashed commits as well.

[DI] Add compiler pass which check extistence of service class and
factory_class name (using class_exists() ) if that is possible - definition is not syntetic or is
not definition via factory method.
Contributor

inso commented Jul 20, 2014

What about adding check for existence of factory method?

Contributor

iltar commented Jul 31, 2014

What if I use a factory method and my class is not a class but an interface? This is perfectly legit right now and can cause a BC break if not allowed any more.

See #11279

services:
    session.flash_bag:
        class:           Symfony\Component\HttpFoundation\Session\FlashBagInterface
        factory_service: session
        factory_method:  getFlashBag
Member

weaverryan commented Aug 4, 2014

@iltar You have a point! Perhaps after checking class_exists, if it is not found, we can also check interface_exists. That would add almost no extra overhead to the compile process (since using an interface as the class doesn't happen very often).

@l3l0 what do you think?

@fabpot fabpot commented on the diff Aug 28, 2014

...dencyInjection/Exception/BadServiceClassException.php
+ */
+
+namespace Symfony\Component\DependencyInjection\Exception;
+
+class BadServiceClassException extends RuntimeException
+{
+ public function __construct($id, $className, $key)
+ {
+ parent::__construct(
+ sprintf(
+ 'Class "%s" not found. Check the spelling on the "%s" configuration for your "%s" service.',
+ $className,
+ $key,
+ $id
+ )
+ );
@fabpot

fabpot Aug 28, 2014

Owner

this should be on one line.

Member

jakzal commented Sep 11, 2014

I like to give interfaces in my factory service definitions, so I'd vote for adding interface_exists().

The reason for configuring an interface instead of a class is proper autocompletion (coding against an interface yada yada yada).

Member

weaverryan commented Sep 22, 2014

The only pending issues that has been added on this PR are

  1. Adding an interface_exists check
  2. Moving the exception message onto one line (fabpot's comment).

@l3l0 Can you make those changes? We're in the last week before 2.6 feature freeze and I would love to see if we can get this considered for a merge :).

Owner

fabpot commented Sep 24, 2014

We must also add support for the new way to register factories. See #12008

Owner

fabpot commented Sep 26, 2014

@l3l0 If you don't have time in the next coming days, I can also finish the PR for you, just let me know.

Owner

fabpot commented Sep 26, 2014

The problem I can see is memory consumption as when compiling the container, it will load all classes referenced in the container. So, we should check that first as I fear it's going to slow down the very first request a lot.

Member

stof commented Sep 26, 2014

@fabpot this will likely have an impact in dev indeed. It needs to be measured.
For the prod, it should be OK as this will happen when running cache:warmup in the CLI

Member

weaverryan commented Oct 1, 2014

Ah, good point about performance! I would love to have this better error message, but certainly not if it significantly increases memory or the loading of the container in the dev environment.

Contributor

l3l0 commented Oct 1, 2014

@fabpot Sure I haven't time recently and probably will be better if you take care about it :)

Owner

fabpot commented Jan 2, 2015

I've just tested the impact on the memory and I think the increase is not acceptable. On a barebone Symfony Standard Edition project, it takes 20MB of memory to run app/console without any cache. After applying the patch, it takes 25MB of memory, so an increase of 5MB (and remember that there are no third-party bundle enabled).

So, except if we only run this when debug is enabled, I'm 👎 on this change.

FWIW, the patch does not work as is as it does not take into account interfaces:

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/CheckServiceClassPass.php b/src/Symfony/Component/DependencyInjection/Compiler/CheckServiceClassPass.php
index 1f0897e..71427b0 100644
--- a/src/Symfony/Component/DependencyInjection/Compiler/CheckServiceClassPass.php
+++ b/src/Symfony/Component/DependencyInjection/Compiler/CheckServiceClassPass.php
@@ -31,11 +31,17 @@ class CheckServiceClassPass implements CompilerPassInterface
     {
         $parameterBag = $container->getParameterBag();
         foreach ($container->getDefinitions() as $id => $definition) {
-            if ($this->allowsToCheckClassExistenceForClass($definition) && !class_exists($parameterBag->resolveValue($definition->getClass()))) {
-                throw new BadServiceClassException($id, $definition->getClass(), 'class');
+            if ($this->allowsToCheckClassExistenceForClass($definition)) {
+                $class = $parameterBag->resolveValue($definition->getClass());
+                if (!class_exists($class) && !interface_exists($class) && !trait_exists($class)) {
+                    throw new BadServiceClassException($id, $definition->getClass(), 'class');
+                }
             }
-            if ($this->allowsToCheckClassExistenceForFactoryClass($definition) && !class_exists($parameterBag->resolveValue($definition->getFactoryClass()))) {
-                throw new BadServiceClassException($id, $definition->getFactoryClass(), 'factory_class');
+            if ($this->allowsToCheckClassExistenceForFactoryClass($definition)) {
+                $class = $parameterBag->resolveValue($definition->getFactoryClass());
+                if (!class_exists($class) && !interface_exists($class) && !trait_exists($class)) {
+                    throw new BadServiceClassException($id, $definition->getFactoryClass(), 'factory_class');
+                }
             }
         }
     }
Member

stof commented Jan 2, 2015

I suggest closing this in favor of https://github.com/matthiasnoback/symfony-service-definition-validator/ which covers this kind of validation already, as well as a bunch of other rules (it checks that the mandatory arguments are passed for instance).
And when using the third-party bundle to run the validation, it is very easy to enable it only in the dev environment.

@fabpot fabpot closed this Jan 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment