Problem/Motivation
Our kernel tests are failing with Symfony 3 because they use the deprecated entity.manager service. Symfony 3 changed the visibility of Symfony\Component\DependencyInjection\ContainerBuilder::createService() from public to private. Drupal uses this class as parent class and wants to override createService(), which is of course not possible anymore because it is private in the parent class in Symfony 3.
We cannot suppress the deprecation warnings easily when using Symfony 3 without overriding critical service container code paths. We want to avoid that for maintainability reasons and rather not mark our entity manager service as deprecated for the time being. #2733279: Drupal and Symfony @deprecated mismatch discusses @depreacted semantics in more details.
Proposed resolution
Do not mark EntityManager as deprecated for now, only in informal comments to avoid Symfony 3 failing our tests.
Remaining tasks
Patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#15 | deprecated-symfony-2726645-15.patch | 7.77 KB | klausi |
Comments
Comment #2
klausiPatch.
Comment #4
dawehnerThis is weird IMHO ... maybe we should remove the entire deprecation check from the container builder.Its just something which doesn't work for us yet.
Our container builder has this workaround to not throw deprecation errors, see
\Drupal\Core\DependencyInjection\ContainerBuilder::createService
Comment #5
klausiAha, when I tested QueryTest it pointed me to the original ContainerBuilder from Symfony, not sure why that one was used instead of the Drupal ContainerBuilder variant.
Comment #6
catchThis already came up in #2611816: Update to symfony 2.8. It might be a case of "don't mark things deprecated until core has no usages".
Comment #7
klausiNow I know why Symfony's ContainerBuilder was called and not ours. The visibility of the method createService() has been changed from public to private. When we update to Symfony 3 our createService() method is completely pointless because it will never be called because the private method on the parent class gets precedence over the public implementation in our class.
Proposed solution: do not mark the entity manager service as deprecated and ditch the createService() method on our ContainerBuilder.
I'll leave the @deprecated tag on the actual EntityManager class for now, since I did not see phpunit test fails locally on Symfony 3.
Comment #9
dawehnerI'm wondering whether we could talk with them to make the method at least protected, so we can actually override it. Removing @deprecated tags for that sounds like a workaround for me.
Comment #10
klausiHm, the problem is that we have a different understanding of "@deprecated" than Symfony. When they deprecate something then they remove all usages of it, that's why they can throw deprecation errors and their own tests still pass.
When Drupal @deprecated EntityManager it was still used all over the place, that's why our own tests fail with deprecation warnings.
I'm not at all convinced that we should override a complex method such as createService() from Symfony just because we want to keep the formal "@deprecated" tag in the comments on our EntityManager class. I think we should be honst and admit that our EntityManager is not deprecated in the Symfony sense and leave a comment on the class.
Comment #11
dawehnerWell the alternative is to fix our testing system to catch those exceptions, similar to https://symfony.com/doc/current/components/phpunit_bridge.html | #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation
Comment #12
catchOpened #2733279: Drupal and Symfony @deprecated mismatch.
Comment #13
klausiThanks for opening and pointing to those, but let's not derail the Symfony 3 update because of that.
I think we should remove the @deprecated tag from EntityManager for now to make progress with the Symfony 3 update without overriding more stuff on ContainerBuilder.
Comment #14
catchYeah that's a good option as a stopgap - is that the only one we're hitting? I remember similar problems with the 2.8 update, but can't find the issue.
Comment #15
klausiYes, EntityManger is the only @deprecated problem we are hitting with the Symfony 3 update, luckily.
Comment #16
dawehnerIn reality its indeed also much less of a problem, because the
@deprecated
tag on theEntityManagerInterface
will not go away.Comment #18
dawehnerComment #20
catchCommitted/pushed to 8.2.x, thanks!
Comment #21
Mile23In core, many legacy code paths relate to
\Drupal::entityManager()
(which is deprecated), which in turn is called by many deprecated functions, particularly inentity.inc
.It's also called by many classes which avoid IoC injection in favor of
\Drupal
, such asEntityType
andModuleInstaller
. My IDE says there are 516 occurrences of\Drupal::entityManager()
.This suggests two paths of action: #2729597: [meta] Replace \Drupal with injected services where appropriate in core and removing usages of deprecated functions/methods in general. I couldn't find it so I made this meta: #2743691: [meta] Remove usages of code marked @deprecated for removal in Drupal 9