Follow-up of #2977107: Use more specific entity.manager services in module .services.yml files.

Feedback from @alexpott there:

+++ b/core/modules/system/src/SystemManager.php
@@ -79,8 +81,14 @@ class SystemManager {
+  public function __construct(ModuleHandlerInterface $module_handler, $request_stack, $menu_tree, $menu_active_trail) {
     $this->moduleHandler = $module_handler;
+    if ($request_stack instanceof EntityManagerInterface) {
+      @trigger_error('Passing the entity.manager service to SystemManager::__construct() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0.. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
+      $request_stack = \Drupal::service('request_stack');
+      $menu_tree = \Drupal::menuTree();
+      $menu_active_trail = \Drupal::service('menu.active_trail');
+    }

We need assert on the interfaces here or we lose type safety. I think the thing to do here is to remove the entity manager type hint and pass a NULL in via the .services file and then file a follow-up for Drupal 9 to fix this. Because then we can make a breaking change... or maybe we have auto-wiring at that point.

I'm not really convinced by that, I'd prefer either this approach (with assert() then for the interface check until we can add the type hints back) or then just removing it. We've done that before, for example in #3001309: Replace usages of UrlGeneratorTrait in non-api classes, which removed an argument from the CommentManager service.

Doing the suggested change in 9.x is IMHO an unnecessary hard break, even though it is very unlikely that this service was subclassed by anyone.. but then we could also just remove it now :)

CommentFileSizeAuthor
#4 3025152-4.patch1.98 KBandypost

Comments

Berdir created an issue. See original summary.

berdir’s picture

Issue summary: View changes
hchonov’s picture

As the probability of someone having subclassed this class is so low, I think that it is fine for us the simply remove the entity manager reference from the constructor.

andypost’s picture

Status: Active » Needs review
StatusFileSize
new1.98 KB

I found no usage in contrib but let's move ahead

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

berdir’s picture

Status: Needs review » Reviewed & tested by the community

I agree. We actually did use the approach I had here in a few cases, but in places that are more likely to be overwritten.

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5128461 and pushed to 8.8.x. Thanks!

  • larowlan committed 5128461 on 8.8.x
    Issue #3025152 by andypost: Remove entityManager constructor argument...

Status: Fixed » Closed (fixed)

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