Remove unused menu active trail service from SystemMenuBlock

Changing status.

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -64,8 +53,7 @@ public static function create(ContainerInterface $container, array $configuratio
diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml

diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
old mode 100644
new mode 100755

This is an unrelated change - this file shouldn't be changing in this path. This is change the mode to 755 - all files in the repo should be 644.

Removed unrelated change mentioned in comment & recreated the same patch.

I've done some thinking about contrib that might be affected by this change - https://www.drupal.org/project/menu_block extends the block but does not change the constructor and does not use the active trail service.

Committed 75194e9 and pushed to 8.4.x. Thanks!

we should revert that since now we do have a use case: https://www.drupal.org/node/2594425

It seems that this change has broken at least Superfish #2907801: Fatal error: Call to a member function getActiveTrailIds() on null in SuperfishBlock.php on line 1462 and it has broken my Entity Submenu Block module too.

Regardless of the number of contrib modules extending SystemMenuBlock, even if there were none, I would still call a change to a public constructor BC breaking, and thus not allowed in a minor release.

I think this commit should be reverted.

Edit: reading https://www.drupal.org/core/d8-bc-policy I learned that that even if inconvenient for some contrib projects, this change is not against BC policy. Still, it would be nice to introduce changes like this in a major release if not actually required for some new core functionality.

As the BC policy states that plugin constructors are considered internal and may change in a minor release, what is the best way to avoid breakage when a plugin constructor is changed in core? In this case for example it seems to make sense to extend SystemMenuBlock to keep the core functionality and add something to it. And if additional container services are required, the recommended way to use dependency injection in plugins is to use ContainerFactoryPluginInterface, which in turn uses the create() and __construct() pattern.

Sorry, I wasn't being very clear: what I meant with "this case" is all these different projects extending SystemMenuBlock, Superfish being just one of them. Entity Submenu Block for example doesn't use $menu_active_trail at all, but it needs to use some container services.

While SystemMenuBlock contains only about 200 lines of code, replicating it in your own class doesn't seem like a very good option to me.

I've understood that \Drupal::service() is only recommended for legacy procedural code, and the recommended way to use dependency injection in plugins is to use ContainerFactoryPluginInterface.

So my question really was about this kind of situation in general, not even specific to SystemMenuBlock, but any contrib project extending any kind of core plugin, that needs to use container services.

To further clarify: I've got nothing against this issue itself, I think it is good to remove something that is not used!

I'm interested in how to best avoid core plugin constructor changes causing potential problems in contrib, since these kind of changes are allowed in the BC policy.

But if there isn't a good solution, maybe the BC policy needs review?

@aleksip asked me look at this issue since 8.4 is out next week. It is problematic that sites using the current Superfish module will break. On the other hand as people have pointed out this change is in accordance with https://www.drupal.org/core/d8-bc-policy and the fix for Superfish etc is to implement their own ::create and ::__construct methods.

I guess the question is do we prioritise this change over broken sites. I think it might be worth revisiting this. I'll ping this issue to the release managers.

So as mentioned the change is allowed, and both modules should really extend BlockBase since it's a base class and SystemMenuBlock isn't (or be prepared to update with minor releases). However for me personally I'd be happy to revert it from 8.4.x given the breakage has been discovered quite late (but leave it in 8.5.x with issues open to update the modules affected).

@bircher suggested that we could use final on concrete plugin classes?

If we used final in an 8.x minor release that would break all extending classes, even if they'd otherwise work (also stops people who know the risks but really want to extend for whatever reason), we should explicitly mark things as @internal in the code base though.

Just confirmed locally that the revert is still clean, so moving to RTBC and bumping priority.

Since I asked the question earlier in this issue, just documenting here for now a great way to avoid breakage, as used in Search API by @drunken-monkey (via @larowlan on Slack).

public static function create($config, $plugin_id, $def, $container) {
  $static = parent::create($config, $plugin_id, $def, $container);
  $static->setEntityTypeManager($container->get('...'));
  return $static;
}

So the basic idea is to avoid calling the parent constructor and use setters!

As per @catch let's revert this for 8.4.x and create a CR to give custom and contrib a chance to update. Here's a patch to ensure nothing breaks on revert.

I've created the change record - https://www.drupal.org/node/2912736

Committed c3e0fe7 and pushed to 8.4.x. Thanks!

Moving back to 8.5.x and fixed.