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?

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!

@pyrech good catch :) I changed names.

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

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

If yes - imho better add console commands for checking.

@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.

when container is compiled (not on every request)

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

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

@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.

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

What about adding check for existence of factory method?

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\FlashBagInterfacefactory_service: sessionfactory_method:  getFlashBag

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

@l3l0 what do you think?

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

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

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

1. Adding aninterface_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 :).

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

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

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.

@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 runningcache:warmup in the CLI

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.

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

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 runapp/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.phpindex 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');+                }             }         }     }

I suggest closing this in favor ofhttps://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.