Comments

Chi created an issue. See original summary.

swarad07’s picture

swarad07’s picture

Status: Active » Needs review
FileSize
2.18 KB

Changing status.

Chi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good for me. Thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

Yogesh Pawar’s picture

Status: Needs work » Needs review
FileSize
2.06 KB

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

Chi’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 75194e9 on 8.4.x
    Issue #2856625 by swarad07, Yogesh Pawar: Remove unused menu active...
Chi’s picture

I've checked the code base of contributed modules and found following modules extending SystemMenuBlock class.

The only module referencing menu.active_trail is Colossal menu.

#2857616: Remove usage of menu.active_trail service

Status: Fixed » Closed (fixed)

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

bircher’s picture

Chi’s picture

@bircher, this should be addressed in #2594425: Add the option in system menu block to "Expand all items in this tree". So far this service is not needed.

aleksip’s picture

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.

aleksip’s picture

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.

Chi’s picture

As the BC policy states that plugin constructors are considered internal and may change in a minor release

Just to be clear, Superfish is not broken because of the change in the parent constructor. It does not override it at all. The error ocurs because of removed protected property menuActiveTrail which Superfish happens to use. However per the BC policy protected properties on a class are also considered @internal.

The simplest fix for this is just replacing $this->menuActiveTrail with \Drupal::service('menu.active_trail').

In this case for example it seems to make sense to extend SystemMenuBlock to keep the core functionality and add something to it.

For me it does not seem so. SystemMenuBlock is pretty small class (~200 lines) while SuperfishBlock is kind of gigantic (~1550). From this point I would go with extending BlockBase instead.

aleksip’s picture

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.

aleksip’s picture

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?

Chi’s picture

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.

Well, the one could simply override ::_constructor and ::create methods but this would produce some redundant code. I think the problem is more general. Extending classes that are not abstract and using or overriding methods and properties that are not part of public API is always a bit tricky. Automated tests can mitigate possible issues with core updates.

alexpott’s picture

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

catch’s picture

Status: Closed (fixed) » Active

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

catch’s picture

Issue tags: +Needs change record
alexpott’s picture

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

catch’s picture

Priority: Minor » Major
Status: Active » Reviewed & tested by the community

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.

aleksip’s picture

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!

alexpott’s picture

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.

alexpott’s picture

Issue tags: -Needs change record

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

  • catch committed c3e0fe7 on 8.4.x
    Issue #2856625 by swarad07, alexpott, Yogesh Pawar, Chi, aleksip: (...
catch’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed c3e0fe7 and pushed to 8.4.x. Thanks!

Moving back to 8.5.x and fixed.

Status: Fixed » Closed (fixed)

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