Fix php 7.4 class resolution behaviour change #1013
Conversation
|
Failures seem to be unrelated. |
|
I'm not sure what this fixes: if the form component is not installed, these definitions are removed when compiling the container, because they are unused. This happens without caring about the classes of the services, which means I'm missing the link with symfony/symfony#32995 |
|
@nicolas-grekas I don't know if it really fixes or not, my goal here was to see how it works in the CI, but may I quote this comment from @TerjeBr : symfony/symfony#32995 (comment) I linked the other issue because I did this in this context. It's more likely linked to symfony/symfony#32396 in reality. As for #32995 it's the same PHP behaviour as #32396 that triggers the fatal errors. I think that in the long run, the only solution to avoid any problems is to be more careful about what is or is not left in the container by bundles, it's their responsability, and removing classes from it before compilation should be (but I might be wrong):
Now it does (in this PR I mean). |
|
The way the definition is currently designed is perfectly fine: it reduces the maintenance burden, by delegating cleanup operations to automated processes. That's the job of computers, letting human brains free from superfluous details. |
I am unsure it does actually, there's conditional code in the extension anyway which cannot be removed, and the only thing that PR does in the end, is just segregating form-related configuration into its own file: each file is smaller, has a finer name expressing what it does, it's subjective I admit, but some might find this easier to maintain.
I don't really think that leaving services to be dropped silently by the container without commenting anything in the XML configuration file is ergonomic, it's open source and anyone literally could end-up debugging its container to understand how the service doesn't reach its compiled service container in the end. A conditional include is not superfluous actually, it's much more expressive and comprehensible for external readers. |
|
There is only one condition here that makes those services unused. But in other places, the decision will depend on a matrix of conditions. We cannot deal with as many files as there are cells in the matrix, all multiplied by the number of bundles that share this concern. This would be wrong. |
Doctrine bundle maintainers are supposed to know what happens, and why they did include the form related code, then even named things "form.*". They're not stupid, they are smart, and they know (at least they should know) why and how this service ends up in the container.
That's why they are bundles, that's why they are maintained by many individuals, because indeed, you alone cannot guess all the possibilities. |
|
Maintainers don't want to have to be smart to be correct. I'm sorry but the approach you're pushing is creating more issues than it solves. |
I'm quite sure everyone is smart enough to understand why one would not include a service if the dependencies are not here. This answer shows you are trying to be smarter than them, but the container is not perfect, and there's some aspects of which greatly lack ergonomics, and people, no matter how hard you try to make it better, will still end up being debugging into it from time to time and bugs will continue to happen in corner cases, and the more the container will be smart and complex, and the more corners there will be. It's easier to be a little smart at the beginning than needing to be extra smart at the end when everything has become out of control. |
|
While well-intended, I don't think opening two discussion threads for the same issue is necessary. I understand where you're coming from @pounard, but as @nicolas-grekas mentioned, this is a simple example, while others are more complicated and heavily depend on the various Symfony component versions that are installed. That said, I'm waiting for symfony/symfony#32995 to come to a resolution before deciding on further steps. Thanks for understanding. |
|
@alcaeus understood, thanks for your time. I'll leave this PoC as-is, it might (or might not) come handy in the future. |
| @@ -345,6 +345,7 @@ protected function ormLoad(array $config, ContainerBuilder $container) | |||
| $loader->load('orm.xml'); | |||
|
|
|||
| if (class_exists(AbstractType::class)) { | |||
| $loader->load('orm-form.xml'); | |||
| $container->getDefinition('form.type.entity')->addTag('kernel.reset', ['method' => 'reset']); | |||
There was a problem hiding this comment.
The tag can be added to orm-form.xml. There is no need anymore to add it programmatically.
|
Can be closed to me. |
|
Closing here: since the issue was fixed upstream, we're not changing this at this time. We'll be splitting this bundle into separate parts for DBAL and ORM, at which time we may decide to better handle such dependencies, but for now I'm not very inclined to split this. |
Following Symfony issue symfony/symfony#32995 from PHP 7.4 a difference in PHP behaviour when resolving classes with missing parent classes or interfaces raise a fatal non-recoverable error.
By moving symfony/form related code into their own service definition file and loading it conditionally, it won't fix the PHP behaviour but at least work around it gracefully.
Please note I didn't tested this code with PHP 7.4 yet.
I know that it might be disruptive to explode the xml files into many smaller ones, but I think it have a nice side effect for maintenance: it decouples definitions for different features that today all stick together. For some people, but that's a subjective opinion, it could improve code readability/maintainability.