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.Problem/Motivation
In #2856625: Remove unused menu active trail service from SystemMenuBlock, the menu_active_trail service was removed from SystemMenuBlock, which this module extends, as it was unused in core.
This results in the following error when running superfish on 8.4:
Error: Call to a member function getActiveTrailIds() on null in .../modules/contrib/superfish/src/Plugin/Block/SuperfishBlock.php on line 903
Proposed resolution
Implement a constructor and create method in SuperfishBlock and inject that service (since it is used there).
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 2896583-09.patch | 2.14 KB | jhedstrom |
Comments
Comment #2
Znak CreditAttribution: Znak commentedComment #3
nick.james CreditAttribution: nick.james at Workday, Inc. commentedComment #4
jhedstromComment #5
jhedstromVerified this fix works as expected with the latest 8.4. Note that it is also backward compatible too, since it only extends the constructor, and injects the menu trail service itself.
Comment #6
vkechagias CreditAttribution: vkechagias at More than Themes commentedPatch in #3 works as expected in Drupal 8.4.0-RC2
Can we get this merged as it will break sites when 8.4 comes out?
Comment #7
jhedstrom8.4 is only a few days away now. Bumping to critical as no sites running this module will work at that point. Note, the fix is BC, so there's no harm in committing it prior to 8.4 either.
Comment #8
jhedstromActually, for 8.4, this was reverted (#2856625: Remove unused menu active trail service from SystemMenuBlock) in order to give contrib some time. Would still be good to get committed in such a way that it works with both 8.4 and 8.5.
Comment #9
jhedstromThis patch should work with both 8.4 and 8.5 (and 8.3, etc).
Comment #11
mehrpadin CreditAttribution: mehrpadin commentedHey there,
Patch applied, thank you.
Comment #12
aleksipHas this patch been tested with 8.5? I don't think it will work in 8.5, because it calls the parent constructor with
$menu_active_trail, which is removed from the parent constructor.The approach in #2907801: Fatal error: Call to a member function getActiveTrailIds() on null in SuperfishBlock.php on line 1462 is better, because it doesn't call the parent constructor at all (see #2856625-25: Remove unused menu active trail service from SystemMenuBlock), so it works in both 8.4 and 8.5.
Comment #13
jhuhta CreditAttribution: jhuhta commentedPHP accepts extra parameters in function calls just fine (so this patch works in 8.5), but do you really think it's ok to take advantage of that? I think we shouldn't be using extra parameters in the SystemMenuBlock constructor call even though they're silently ignored in practice.
I'd check the other patch in #2907801 as @aleksip already mentioned.
Comment #14
aleksipOops, I didn't have my PHP brain on, sorry about that! Still, the constructor-less solution would be more future-proof.
Comment #15
jhedstromI've manually tested this approach in 8.4 and 8.5 (it would be nice if this module had tests to run automatically, but that's an issue for another day)
Comment #16
pinch' CreditAttribution: pinch' as a volunteer commentedworks in 8.5
Comment #17
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedWould be great to have a new stable release soon - more and more people are starting to use 8.5 now
Comment #18
mehrpadin CreditAttribution: mehrpadin commentedHey everybody,
I just did it (v1.2) thank you all for testing and everything. I'll see if I can make some time for creating the tests etc.
Comment #19
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks @mehrpadin!
Looks like this is now fixed not needs review. If anyone wants to improve #12-14 presumably it would make sense to raise a new issue with a patch that applies on current HEAD.