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

CommentFileSizeAuthor
#9 2896583-09.patch2.14 KBjhedstrom
#9 interdiff-2896583-03-09.txt727 bytesjhedstrom
#3 superfish-add-missing-service-2896583-3-D8.patch2.12 KBnick.james

Comments

jhedstrom created an issue. See original summary.

znak’s picture

znak commented
Assigned: Unassigned » znak
nick.james’s picture

nick.james commented
StatusFileSize
new superfish-add-missing-service-2896583-3-D8.patch2.12 KB
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Active » Needs review
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Reviewed & tested by the community

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

vkechagias’s picture

vkechagias commented

Patch 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?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Priority: Major » Critical

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

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Priority: Critical » Major

Actually, 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.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new interdiff-2896583-03-09.txt727 bytes
new 2896583-09.patch2.14 KB

This patch should work with both 8.4 and 8.5 (and 8.3, etc).

  • mehrpadin committed da8c250 on 8.x-1.x authored by jhedstrom 
    Issue #2896583 by jhedstrom, nick.james: Menu active trail service...
mehrpadin’s picture

mehrpadin commented

Hey there,

Patch applied, thank you.

aleksip’s picture

aleksip
Primary language Finnish
Location Finland
commented

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

jhuhta’s picture

jhuhta commented

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

aleksip’s picture

aleksip
Primary language Finnish
Location Finland
commented

Oops, I didn't have my PHP brain on, sorry about that! Still, the constructor-less solution would be more future-proof.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

I'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)

pinch'’s picture

pinch' commented

works in 8.5

adamps’s picture

adamps commented

Would be great to have a new stable release soon - more and more people are starting to use 8.5 now

mehrpadin’s picture

mehrpadin commented

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

adamps’s picture

adamps commented
Assigned: znak » Unassigned
Status: Needs review » Fixed

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

Status: Fixed » Closed (fixed)

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