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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
FileSize
1.56 KB

Patch.

Status: Needs review » Needs work

The last submitted patch, 2: deprecated-symfony-2726645-2.patch, failed testing.

dawehner’s picture

This 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

klausi’s picture

Aha, 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.

catch’s picture

This 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".

klausi’s picture

Status: Needs work » Needs review
FileSize
7.03 KB
7.82 KB

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

Status: Needs review » Needs work

The last submitted patch, 7: deprecated-symfony-2726645-7.patch, failed testing.

dawehner’s picture

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

I'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.

klausi’s picture

Hm, 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.

dawehner’s picture

Hm, 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.

Well 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

catch’s picture

klausi’s picture

Title: Remove deprecated meta info for Symfony 3 update » Do not @deprecate EntityManager until all usages are removed (helps the Symfony 3 update)
Issue summary: View changes

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

catch’s picture

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

klausi’s picture

Status: Needs work » Needs review
FileSize
7.77 KB
754 bytes

Yes, EntityManger is the only @deprecated problem we are hitting with the Symfony 3 update, luckily.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

In reality its indeed also much less of a problem, because the @deprecated tag on the EntityManagerInterface will not go away.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: deprecated-symfony-2726645-15.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

  • catch committed 4c30de3 on 8.2.x
    Issue #2726645 by klausi: Do not @deprecate EntityManager until all...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x, thanks!

Mile23’s picture

+ * Deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. We cannot
+ * use the deprecated PHPDoc tag because this service class is still used in
+ * legacy code paths. Symfony would fail test cases with deprecation warnings.

In core, many legacy code paths relate to \Drupal::entityManager() (which is deprecated), which in turn is called by many deprecated functions, particularly in entity.inc.

It's also called by many classes which avoid IoC injection in favor of \Drupal, such as EntityType and ModuleInstaller. 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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.