Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Run theResolveFactoryClassPass whenlint:container builds the container from a dump#50988
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lugosium commentedJul 15, 2023
I confirm your PR fix the issue 👍 🙏 Thanks a lot. |
nicolas-grekas commentedJul 15, 2023
Did you consider enabling the pass on the command? That'd make more sense to me because if we do this, then we should do the same in PHP dumper, and the pass becomes useless. Also, this misses the check for $class that the pass has. WDYT ? |
MatTheCat commentedJul 15, 2023
TBH I didn’t consider updating the command because it looks like it must work without any compiler pass.
I may have missed something because I don’t understand why 🤔 |
nicolas-grekas commentedJul 15, 2023
It never did before we introduced ResolveFactoryClassPass so I don't think so. |
MatTheCat commentedJul 15, 2023
Is this what you’re asking for? diff --git a/src/Symfony/Bundle/FrameworkBundle/Command/ContainerLintCommand.php b/src/Symfony/Bundle/FrameworkBundle/Command/ContainerLintCommand.php--- a/src/Symfony/Bundle/FrameworkBundle/Command/ContainerLintCommand.php(revision 6f2e603c176724873f92942ea7461150d0e32040)+++ b/src/Symfony/Bundle/FrameworkBundle/Command/ContainerLintCommand.php(date 1689424425782)@@ -20,6 +20,7 @@ use Symfony\Component\Console\Style\SymfonyStyle; use Symfony\Component\DependencyInjection\Compiler\CheckTypeDeclarationsPass; use Symfony\Component\DependencyInjection\Compiler\PassConfig;+use Symfony\Component\DependencyInjection\Compiler\ResolveFactoryClassPass; use Symfony\Component\DependencyInjection\Container; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;@@ -120,7 +121,7 @@ } $container->getCompilerPassConfig()->setBeforeOptimizationPasses([]);- $container->getCompilerPassConfig()->setOptimizationPasses([]);+ $container->getCompilerPassConfig()->setOptimizationPasses([new ResolveFactoryClassPass()]); $container->getCompilerPassConfig()->setBeforeRemovingPasses([]); } It feels weird needing this particular pass to build the container from a dump. |
fdd8a62 tod8afb57Comparenicolas-grekas commentedJul 15, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes that's what I mean. Dealing with th dumped XML requires special care, this makes sense to me. At least more than making the pass half-needeed. I prefer keeping the symmetry between the dumped container and a compiled container builder. |
…ner` builds the container from a dump
d8afb57 to5cf4b63Comparenull factory class inContainerBuilder::createServiceResolveFactoryClassPass whenlint:container builds the container from a dumpMatTheCat commentedJul 15, 2023
Okay PR updated, thanks for the feedback 🙏 |
ResolveFactoryClassPass whenlint:container builds the container from a dumpResolveFactoryClassPass whenlint:container builds the container from a dumpnicolas-grekas commentedJul 16, 2023
Thank you@MatTheCat. |
ResolveFactoryClassPass whenlint:container builds the container from a dumpResolveFactoryClassPass whenlint:container builds the container from a dump
Uh oh!
There was an error while loading.Please reload this page.
#49665 replaced the
factorynode by aconstructorattribute in the XML and YAML dumper when the factory’s class is the same as the definition’s. The corresponding loader then creates a definition where the factory class isnull.As the
ResolveFactoryClassPasswill not run when thelint:containercommand builds the container from an XML dump, such factories would makeContainerBuilder::createServicecrash. This PR adds this compiler pass to the list.