Hi,

I may be jumping the gun a bit, but I'm having some issues with the module on the Drupal 9 alpha. I'm getting the following error:

ArgumentCountError: Too few arguments to function Drupal\system\Plugin\Block\SystemMenuBlock::__construct(), 4 passed in /srv/bindings/5054caa179a84771a26251400e792dbb/code/modules/contrib/menu_block/src/Plugin/Block/MenuBlock.php on line 73 and exactly 5 expected in Drupal\system\Plugin\Block\SystemMenuBlock->__construct() (line 57 of /srv/bindings/5054caa179a84771a26251400e792dbb/code/core/modules/system/src/Plugin/Block/SystemMenuBlock.php)

I've also applied the patch in #99 here:

https://www.drupal.org/project/menu_block/issues/2809699

All in all, the module is causing two errors:

1. I can't seem to place a menu block in the Block Layout screen. I just get the pinwheel for a split second before it does nothing. I checked the log and the error at the top is what I've got.

2. The advanced options in the patch in #99 aren't appearing. Those options are great and I'm hoping to use them on a D9 site. Why they're not appearing and if they're related to the error I'm getting, I can't say.

The only Drupal 9 deprecation I'm getting in my D8 site is the core requirement, which I added in the info.yml file before trying the module out on D9.

Hopefully someone who knows more than I do can shine some light on this issue.

CommentFileSizeAuthor
#11 3115436.10_11.interdiff.txt449 bytesdww
#11 3115436-11.patch2.37 KBdww
#10 3115436.5_10.interdiff.txt542 bytesdww
#10 3115436-10.patch2.37 KBdww
#5 3115436-5.patch2.27 KBjungle

Comments

mattbloomfield created an issue. See original summary.

mattbloomfield’s picture

mattbloomfield commented

I've done some digging, and I think it might be this difference in the core System Menu Block:

D8.8:

public function __construct(array $configuration, $plugin_id, $plugin_definition, MenuLinkTreeInterface $menu_tree, MenuActiveTrailInterface $menu_active_trail = NULL) {
parent::__construct($configuration, $plugin_id, $plugin_definition);
$this->menuTree = $menu_tree;
if ($menu_active_trail === NULL) {
@trigger_error('The menu.active_trail service must be passed to SystemMenuBlock::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/2669550.', E_USER_DEPRECATED);
$menu_active_trail = \Drupal::service('menu.active_trail');
}
$this->menuActiveTrail = $menu_active_trail;
}

/**
* {@inheritdoc}
*/
public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
return new static(
$configuration,
$plugin_id,
$plugin_definition,
$container->get('menu.link_tree'),
$container->get('menu.active_trail')
);
}

D9:

public function __construct(array $configuration, $plugin_id, $plugin_definition, MenuLinkTreeInterface $menu_tree, MenuActiveTrailInterface $menu_active_trail) {
parent::__construct($configuration, $plugin_id, $plugin_definition);
$this->menuTree = $menu_tree;
$this->menuActiveTrail = $menu_active_trail;
}

/**
* {@inheritdoc}
*/
public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
return new static(
$configuration,
$plugin_id,
$plugin_definition,
$container->get('menu.link_tree'),
$container->get('menu.active_trail')
);
}

And here's what I've got in the menu block module in MenuBlock.php:

public function __construct(array $configuration, $plugin_id, array $plugin_definition, MenuLinkTreeInterface $menu_tree, MenuActiveTrailInterface $active_trail, EntityTypeManagerInterface $entity_type_manager) {
parent::__construct($configuration, $plugin_id, $plugin_definition, $menu_tree);
$this->menuActiveTrail = $active_trail;
$this->entityTypeManager = $entity_type_manager;
}

I'm not sure what that should be updated to, though.

dww’s picture

dww
we/he/they
commented

#2809699-111: Add configuration options for dynamic block titles should fix all of this (although I haven't tested on D9 myself, yet).

I could split out the parts that fix the handling of menuActiveTrail to a separate patch for this issue if the maintainers prefer for scope management/focus.

dww’s picture

dww
we/he/they
commented
Title: Potential Drupal 9 issue » MenuBlock handling of activeMenuTrail is deprecated (and unnecessary)

Leaving this here as a more appropriate place for the reference:
https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...

Fixing the title to be more accurate.

Also, if we're going to fix the deprecated stuff here, and potentially refactor to not have to worry about constructor changes in the future, we should also fully remove our protected copy of that service pointer. SystemMenuBlock already has it, so we're managing our own duplicate copy for nothing. See #2809699-110: Add configuration options for dynamic block titles for more.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Active » Needs review
StatusFileSize
new 3115436-5.patch2.27 KB

Only refactored src/Plugin/Block/MenuBlock according to the blog post shared in #4

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented 
+++ b/src/Plugin/Block/MenuBlock.php
@@ -23,6 +25,32 @@ use Drupal\system\Plugin\Block\SystemMenuBlock;
+  public function setMenuParentFormSelector(MenuParentFormSelectorInterface $menu_parent_form_selector) {
+    $this->menuParentFormSelector = $menu_parent_form_selector;
+  }

Just realised, maybe we should return $this to support method chaining.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Assigned: Unassigned » jungle
Status: Needs review » Needs work

Also, please note that MenuBlock shouldn't have a menuActiveTrail protected member anymore, either. That's cruft and should be removed as part of such a refactoring.

Back to NW, haven't done this.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Assigned: jungle » Unassigned

It takes longer than expected, unassign myself to allow others working on this as it's critical.

dww’s picture

dww
we/he/they
commented
Title: MenuBlock handling of activeMenuTrail is deprecated (and unnecessary) » Use proper dependency injection and protect menu_block from changes to the SystemMenuBlock constructor
Category: Bug report » Task
Priority: Critical » Normal
Status: Needs work » Reviewed & tested by the community

Re: #4 -- Sorry, I was getting confused since I was looking at my copy of this code with #2809699: Add configuration options for dynamic block titles already applied. The copy of menu_block in Git doesn't have activeMenuTrail at all. ;)

Giving this issue a more accurate title/scope, fixing the priority/category, and marking RTBC. #5 is sufficient for this issue.

Let's get this in, then we can focus on #2809699.

Thanks!
-Derek

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3115436-10.patch2.37 KB
new 3115436.5_10.interdiff.txt542 bytes

This addresses #6 and adds return $this.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 3115436-11.patch2.37 KB
new 3115436.10_11.interdiff.txt449 bytes

Oh right, code standards wants a blank line before @return

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Assigned: Unassigned » kristen pol
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Thanks for the update.

1) Confirmed that the changes in #11 are for returning $this as noted in #6. Note that some core code adds a description for * @return $this and some do not.

2) Diffed the patches from #5 and #11 to double check the changes were only what was in the interdiff.

3) Double checked the other code in the patch and seems ok. I noticed that the naming in the patch is different than what's used in core, e.g.

./core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php:  protected $menuParentSelector;
./core/modules/menu_ui/src/Controller/MenuController.php:  protected $menuParentSelector;
./core/modules/menu_link_content/src/Form/MenuLinkContentForm.php:  protected $menuParentSelector;

whereas the patch is using:

  protected $menuParentFormSelector;

though I'd argue that the patch is following a more standard naming pattern than the core code in this case. Likewise for:

menu_parent_selector vs. menu_parent_form_selector

4) Patch applied cleanly:

[mac:kristen:menu_block8]$ patch -p1 < 3115436-11.patch 
patching file src/Plugin/Block/MenuBlock.php

5) Searched the code for other references to menu_parent_form_selector and there were none other than what's handled in the patch.

6) Tests pass.

7) Not sure what's the best way to manually test this.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Assigned: kristen pol » Unassigned
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Related issues: +#3129184: Remove description from @return $this

When you do @return $this, do not provide a description like "The called object" -- just take that line out.

-- from 2711739.#18.3 by @jhodgdon, a leader of the documentation team

Re#13.1, Checked API documentation and comment standards, it's an undocumented convention IMO.

BTW, an issue just created #3129184: Remove description from @return $this

Thanks!

dww’s picture

dww
we/he/they
commented
Related issues: +#2968049: Dependency Injection issue

Sadly, we all missed #2968049: Dependency Injection issue when working on this, so we've now got 2 issues for the same thing. :(

Probably this should be marked duplicate and we should move back over there. Or we can commit/fix this, and simply mark that one fixed, too. ;)

  • joelpittet committed 23228c6 on 8.x-1.x authored by dww 
    Issue #3115436 by dww, jungle, mattbloomfield, Kristen Pol: Use proper...
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Fixed

Thank you for the great review @Kristen Pol and the patch @dww. Also nice follow-up @jungle! I've committed this to the dev release and I did a quick manual test... just in case.

I'll see if I can credit the other issue too

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

@joelpittet Thanks! I see commit credit for:

dww, jungle, mattbloomfield, Kristen Pol

but only issue credit for:

dww, jungle, joelpittet

Would add issue credit for me and mattbloomfield, please :)

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Tried to check and save the checkboxes, but did not get saved, let's wait for maintainers :p

Status: Fixed » Closed (fixed)

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