Make SystemMenuBlock's new constructor argument dependency optional

You could also do it like this:

$this->menuActiveTrail = $menu_active_trail ?: \Drupal::service('menu.active_trail');

But then you can't do a @trigger_error(), which we might want to add as well.

Great.

Not sure if we need the change record referenced, since the necessary change is obvious and the change record does actually not explain it further anyway. We have a rule to always have it, but seems strange for those trivial constructor changes.

RTBC++ to get this change in, the list of contrib affected in related issue

I didn't think constructors or plugins were part of the API, but perhaps this should be a special case if it's extended a lot in contrib.

For these kind of changes, do we require the code path be triggered at least once in a test?

The thing is this work then needs to be undone sometime. Yes the @trigger_error() makes it easier to find but there's even less disruption if people extend plugins doing some thing like https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...

For me, contrib / custom doing something like:

class CorePlugin {
    protected $service;

    public function __construct($service) {
        $this->service = $service;
    }
    
    public static function create() {
        return new static('foo');
    }
}

class ContribPlugin extends CorePlugin {
    protected $anotherService;
    
    public function getAnotherService() {
        return $this->anotherService;
    }
    
    public static function create() {
        $instance = parent::create();
        $instance->anotherService = 'bar';
        return $instance;
    }
}

// Returns 'bar'
var_dump(B::create()->getAnotherService());

@alexpott: Interesting, I think I like that approach. But nobody is using that yet, including core.

What about getting this in, because there are contrib modules out there that are now broken in 8.7 and then creating a documentation page on backwards and forwards compatible ways to subclass services as well as plugins/controllers/.. and start doing that in core?

For services, setter-based injection works well, which I've documented a while ago here: https://www.md-systems.ch/de/blog/techblog/2016/12/17/how-to-safely-inje....

So I think to be friendly we should do this patch. In future I think we should seriously consider enforcing the non-extendability of plugin classes by using final so that people are force to use composition over inheritance to extend from core. This will solve lots of these type of issues and, whilst it will cost more upfront, will put the Drupal eco-system in a more maintainable and upgradable state.

On of the problems of doing this is that we're introducing logic that we need to add a test for. Here's one.

Ughs meant to make the operator === instead of ==

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

Committed 449e2e3 and pushed to 8.7.x. Thanks!

Lee posted one as well https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl... Don't mind me @alexpott shared it already in #10.