Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#26 | 2856625-26.patch | 2.06 KB | alexpott |
#6 | remove_unused_menu-2856625-6.patch | 2.06 KB | yogeshmpawar |
#3 | remove_unused_menu-2856625-3.patch | 2.18 KB | swarad07 |
#2 | remove_unused_menu-2856625-2.patch | 2.18 KB | swarad07 |
Comments
Comment #2
swarad07Comment #3
swarad07Changing status.
Comment #4
Chi CreditAttribution: Chi commentedLooks good for me. Thanks.
Comment #5
alexpottThis 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.
Comment #6
yogeshmpawarRemoved unrelated change mentioned in comment & recreated the same patch.
Comment #7
Chi CreditAttribution: Chi commentedComment #8
alexpottI'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!
Comment #10
Chi CreditAttribution: Chi commentedI'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
Comment #12
bircherwe should revert that since now we do have a use case: https://www.drupal.org/node/2594425
Comment #13
Chi CreditAttribution: Chi commented@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.
Comment #14
aleksipIt 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.
Comment #15
aleksipAs 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 useContainerFactoryPluginInterface
, which in turn uses thecreate()
and__construct()
pattern.Comment #16
Chi CreditAttribution: Chi commentedJust 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').
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.
Comment #17
aleksipSorry, 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 useContainerFactoryPluginInterface
.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.Comment #18
aleksipTo 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?
Comment #19
Chi CreditAttribution: Chi commentedWell, 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.
Comment #20
alexpott@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.
Comment #21
catchSo 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).
Comment #22
catchComment #23
alexpott@bircher suggested that we could use final on concrete plugin classes?
Comment #24
catchIf 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.
Comment #25
aleksipSince 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).
So the basic idea is to avoid calling the parent constructor and use setters!
Comment #26
alexpottAs 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.
Comment #27
alexpottI've created the change record - https://www.drupal.org/node/2912736
Comment #29
catchCommitted c3e0fe7 and pushed to 8.4.x. Thanks!
Moving back to 8.5.x and fixed.