Dependency Injection issue

The patch is empty

Thanks that looks nice, I'm just checking with someone on a bit of it.

The menu.link_tree service is also being injected, but it's not used by any code changed in the patch. Is there a further change to use that service, or could it be removed from the constructor?

We should create a patch or separate issue for 8.x-1.5 version as well.

1. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -18,6 +21,53 @@ use Drupal\system\Plugin\Block\SystemMenuBlock;
   public function __construct(array $configuration, $plugin_id, $plugin_definition, MenuLinkTreeInterface $menu_tree, MenuParentFormSelector $menuParentFormSelector) {
   

   Let's cast MenuParentFormSelectorInterface rather than the implementation class

2. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -18,6 +21,53 @@ use Drupal\system\Plugin\Block\SystemMenuBlock;
   +  /**
   +   * The menu link tree service.
   +   *
   +   * @var \Drupal\Core\Menu\MenuLinkTreeInterface
   +   */
   +  protected $menuTree;
   

   This property is already defined on the SystemMenuBlock, so you do not have to repeat it here

3. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -18,6 +21,53 @@ use Drupal\system\Plugin\Block\SystemMenuBlock;
   +   * Constructs a new SystemMenuBlock.
   

   This class 'SystemMenuBlock' does not match the current class 'MenuBlock'

4. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -18,6 +21,53 @@ use Drupal\system\Plugin\Block\SystemMenuBlock;
   +    $this->menuTree = $menu_tree;
   

   This assignment is redundant with the assignment in SystemMenuBlock and can be removed.

Providing new patch with the changes suggested by @idebr. Please review it.

There was a small typo on the docblock that had a semicolon after the interface in #9 but removed that.
And the menu_tree still needs to be in the create method and __construct, just don't need the protected property or to assign it again.

I've committed an updated version of #9 Thanks for your help on this.

Damn, the constructor changed upstream: #2594425: Add the option in system menu block to "Expand all items in this tree"

I'm not sure how to resolve this... with a changed constructor this breaks BC.

Ok the simplest approach is to revert this for now. I created a patch in core to make that new argument optional, but for now if we avoid extending the constructor/create method we keep this working without forcing people to upgrade to Drupal 8.7

@Berdir wrote an article about the problem:
https://www.md-systems.ch/de/blog/techblog/2016/12/17/how-to-safely-inje...

Ok with some suggestions from @drclaw, got something that seems to work without having to deal with the SystemMenuBlock's changing constructor (there is another issue in the queue that want's to change this constructor too #2854013: Allow SystemMenuBlock tree manipulators to be altered).

Please have a go at reviewing and testing this.

From @Sam152 a blog post on the subject too:
https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...

Added a couple changes in there that weren't needed.

@joelpittet is there any reason that you are not going for the solution in https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...?

@alexpott, I commented on the blog and when I tried to implement it the create method still needs the right number of arguments when calling parent::createnew static() and thus the new argument still breaks BC as it stands because it would need to exist. This decorator, although more work, works regardless of what core does to the constructor (though may break if a new public method is added to that block if it's needed...

EDIT I merged the approach wrong and forgot the key parent::create interface doesn't change.

Looked at it again and it does work, just PHPStorm complains the method doesn't exist on the parent class, but new static() creates an instance of the MenuBlock class!

🤯

I like this approach a lot!

Only a minor annoyance

According to the comment from @alexpott, you don't even need a setter method, you can just set the property because it's an instance of your class.

Actually, I'm taking that back, because if you don't have a setter, you can't mock it from a unit test.

In the unit test a setter would only be required if you're instantiating the class directly, instead of using create() and passing it a container with the mock service.

Which is exactly what unit tests are meant to do IMHO, going through a container should only be done if necessary, and points out when we are using a service locator pattern instead of actual dependency injection :)

You could have a protected setter and use reflection to allow access. Or you could sub-class for the test. Or you could use the composition pattern as @joelpittet did originally. I think if unit testing is super important to you then doing composition is going to save you a few headaches. I think the point is maybe that we just don't support extending from concrete plugins so whatever you are doing it is likely there is a compromise. That said - core can be nice to people so I'm going to commit the upstream issue. I do think that come D9 we should have the final debate and probably enforce the don't extend a concrete plugin via the language construct because that would make life easier for everyone when things changes - as they always do.

@alexpott that works for tests but does not for hook plugin alter, when you override plugin definition. Menu block givev no way to alter manipulators so every contrib have to override constructor

@andypost then just copy all the code and replace the class. We need to stop treating all-the-things as extension points or core becomes impossible to change.

For me after thinking about this some more I really like #18. Here's a little review of that patch.

+++ b/src/Plugin/Block/MenuBlock.php
@@ -30,7 +107,7 @@ class MenuBlock extends SystemMenuBlock {
+    $form = $this->systemMenuBlock->blockForm($form, $form_state);

@@ -127,6 +202,7 @@ class MenuBlock extends SystemMenuBlock {
   public function blockSubmit($form, FormStateInterface $form_state) {
+    $this->configuration['expand_all_items'] = $form_state->getValue('expand_all_items');
     $this->configuration['follow'] = $form_state->getValue('follow');
     $this->configuration['follow_parent'] = $form_state->getValue('follow_parent');

Could call $this->systemMenuBlock->blockSubmit($form, $form_state); here.

Just opened #3019332: Use final to define classes that are NOT extension points which was inspired by this and other issues.

We need to stop treating all-the-things as extension points or core becomes impossible to change.

🙏

Thanks for opening #3019332: Use final to define classes that are NOT extension points!

Thinking about #18 some more - you could inject an instance of the SystemMenuBlock plugin. It do the instance creation in ::create() that way the dependency on the concrete SystemMenuBlock plugin is very explicit because it is in your constructor.

@alexpott, @gapple, and @Berdir thanks for your thoughts around this!

I'm leaning towards #21 because I wouldn't have to decorate any new public methods on that class either (similar to new param changes in constructor). Also I've seen contrib, in two other cases this week trying to override a constructor like this and want to have a succinct suggestion for them. I see the architectural ideas behind #18 may be better for testing but it seems #19 would be easier to maintain.

Obviously still a bit torn/paralyzed in my thoughts to pull the commit trigger.😖

Eeek, I didn't see this issue when #3115436: Use proper dependency injection and protect menu_block from changes to the SystemMenuBlock constructor was created. That's now a basically duplicate solution to this same problem. Alas. :(

Merged with the committed #3115436: Use proper dependency injection and protect menu_block from changes to the SystemMenuBlock constructor version to incorporate the better parts of both, including removing the return of $this on the setter injection and to credit all the people who helped here.