Enforce block plugins returning an array

Inspired by BreadcrumbManager can we just do something like this? There is a null check further down, but should we even allow NULL returns?

We can deprecate ::build() without a return type and add it to the interface in Drupal 10.

This means we have to add return types across all of core, but not sure what to do about the possibility of contrib/custom code extending core blocks and overriding ::build()?

+++ b/core/lib/Drupal/Core/Block/BlockManager.php
@@ -65,6 +66,20 @@ protected function getType() {
+    if ($class) {

Apart from the unit tests is there a valid case where $class can still be NULL by the time we get here?

+++ b/core/tests/Drupal/Tests/Core/Block/BlockManagerTest.php
@@ -107,4 +123,47 @@ public function testHandlePluginNotFound() {
+    $expected = [];
+    $definitions = $this->blockManager->getDefinitions();
+    $this->assertSame($expected, $definitions);

I did a double take at this as I thought $expected should contain block1, then I realised it's because of the dummy provider, is this worth changing?

Otherwise the patch and the CR both look good, unsure if I can RTBC though.

Updated IS.

I love this. Typed returns++

Any reason this shouldn't be RTBC?

+++ b/core/tests/Drupal/Tests/Core/Block/BlockManagerTest.php
@@ -107,4 +124,49 @@ public function testHandlePluginNotFound() {
+    // Overwrite the definitions from ::setUp() to have a block thagt does not

Nit: thagt -> that

(spellchecker in core won't let this be committed now!)

#16: #3122096: Support code spell checking

Just ++ing the rtbc. No functional changes.

+++ b/core/lib/Drupal/Core/Block/BlockManager.php
@@ -2,6 +2,8 @@
+use Drupal\Component\Plugin\Definition\PluginDefinitionInterface;

Unused use statement removed. Stay RTBC.

There are no changes to BlockPluginInterface here. Even if we can't add the return type hint in 9.1.x, we should document there that it will be added in 10.0.x I think?

Think we're back to RTBC.

So this will break any contrib or custom code that extends block and overrides a build method - see https://3v4l.org/rbmaP

I have argued in the past that we should mark all blocks as final - see #3019332: Use final to define classes that are NOT extension points - unfortunately this has proved contentious but if we don't do that making changing like this is always going to be hard. Given this is the first issue in runtime code about return typehints (we have PHPUnit requiring them in the test system) I think we need release manager input on the way forward. I think we should mark all core blocks as final because then adding return typehints to other part of of the interface in a similar fashion will be simpler and there is then only one "break".

Alternatively do we need to warn contrib with another deprecation?

My initial thoughts are that at least the typehint break is a one-line automatic change, and could be paired with a wakeup call change record about not extending blocks.

Making all of the core blocks final is going to make things rough to work around.

Yes this is my feeling as well. If you extend a block, you might or might not be OK, and you might or might not need to make a one line change. If we made the blocks final in a minor, then every extending class immediately gets a hard break - it's something we should only do in a major release IMO (apart from the further argument about whether to do it or not in general).

Also adding the return type hint works fine in the other direction - so a module that updates for 9.1.x will be compatible with 9.0.x without having to branch or raise minimum requirements etc.

> Except that we have abstract block classes which are totally okay to extend from. My reflection is a bit rusty, can you instead check that the parent is abstract?

Yep, we can, let's see how this fares.

I even wonder if we should move this up a layer to DefaultPluginManager - ie. we should extend this to all plugins, not just blocks.

Fixing test failures.

Wow, one very large patch!

FWIW: I just did a dry run on 9.0.x, the patch applies cleanly so I triggered tests on 9.0.x also looks like it doesn't apply to 9.0.x after all, for some reason.

Ok, so a few points here:

1. I hate the idea of making any plugin core ships final. Maybe one or two here and there for a particularly good reason, but the whole idea was always that we wouldn't have to replicate the logic in hooks because OO allowed inheritance. Functionally, declaring plugin classes final makes them no better for down-stream developers than having D7-style hooks would. If they want similar functionality, they'll have to copy and maintain all the same methods/code. Yuck.
2. Adding the typed return breaks downstream subclasses. I love this patch, however I hadn't considered this aspect. I think we (Drupal) may need to have a special BC consideration for introducing typed returns since there's no good way to prevent this. Initially I'd hoped that not putting it on the interface (yet) would be enough, but subclasses will break the run time. All of that to say, have we discussed this previously wrt our BC policy? Can someone point me at that if we have?
3. There's been talk for a while that we could/should be producing rector rules for these sorts of changes. Is there any potential appetite for doing that at a point release instead of a major release? The fix for this is so easy from a code perspective and affects so few contrib modules. It might be an interesting pilot for this problem space.
4. Finally, given my second point, I'm not sure I see the point of the trigger_error for blocks that are subclassing others. As I said in my first point, we definitely envisioned people subclassing core plugins to make minor modifications. Whatever the case, the mere presence of the subclass in the php run time will break it. It'd never make it this far.

Thoughts?

Updated deprecated @expectedDeprecation annotation.

I couldn't get any of the previous patches to apply on D9.07:

error: core/modules/node/tests/modules/node_block_test/src/Plugin/Block/NodeContextTestBlock.php: No such file or directory

For those looking for a quick solution the following patch works: #3121395: Fatal error after add Main page content block layout builder

Rerolled patch for 9.0.

#45 fixes the problem where the Main Content block cannot be added to the Layout Builder variant for Panels Everywhere, as discussed here: #3143487: Not compatible with "Layout Builder" variant as site template (Workaround found).

However, as mentioned in #4, it does indeed cause problems with certain contrib modules. Superfish has the following error message after applying the patch (See: https://www.drupal.org/project/superfish/issues/2951400#comment-13919952):

PHP Fatal error: Declaration of Drupal\superfish\Plugin\Block\SuperfishBlock::build() must be compatible with Drupal\system\Plugin\Block\SystemMenuBlock::build(): array in /modules/contrib/superfish/src/Plugin/Block/SuperfishBlock.php on line 24

Needs to update patch deprecation for 9.2.0

Hi,

Creating a patch as suggested in #47 comment.

Please review the patch.

Thanks.

#50 patch is not applied to 9.2.0 version. I've created a new one.

This seems straightforward to me.

Ah, sorry -- I found one more thing that might be a condition to defend against.

I'm definitely concerned about the part where we add typehints to existing core classes. Plugin implementations are one of those technically internal, but nonetheless tempting to subclass, internal APIs. I also agree with:

Looking at SystemMenuBlock, there is a LOT of useful stuff in there and I don't blame them for overriding it. If that were to be marked final, it'd be considerate of us to provide a service or a trait to encapsulate that code.

@Berdir expresses related perspective in #3019332-3: Use final to define classes that are NOT extension points.

Furthermore, we have an expectation of providing deprecations rather than BC breaks even for internal APIs. Adding a return typehint to a parent class definitely qualifies. There may only be five usages in contrib, but there could be many more in custom code, and plugins are the kind of thing that's likely to get reused and tweaked there.

Both release managers have already indicated they are not presently comfortable adding final to things like plugins and controllers (in the referenced issue).

I think we should split this issue up, which @tim.plunkett also suggested in chat.

1. Stuff to keep or add:
   • Raising deprecations about an incoming or outgoing data type that doesn't match our expectation is absolutely something we can commit without hesitation.
   • We can also add the typehints directly in D9 for the test fixtures.
   • We can even raise the same deprecation as in the patch for any direct implementations, and even sub-subclasses of core's current classes. This will both prevent us from introducing new implementations in core without the typehint (see #3154762: [policy, no patch] All new Drupal 10 code should use appropriate type hints wherever possible) and also ready contrib for us to add the typehint to the core implementations and the parent class in D10.
2. The discussion and deprecation around making it impossible to subclass block plugins should also be moved to a separate related issue; it's not in scope and not necessarily even desirable. (We can discuss.)
3. I like the idea of providing a trait or such for the useful bits in the existing system blocks; let's file an issue for that as well?
4. "Require a typehint in D10 and deprecate it for child implementations" is going to be something we do over and over. It's probably worth providing as a generic core API. Also its own issue.

Hi! Is there a patch for 9.3?

The patch no longer applied to 9.4.8. It was only failing on core/tests/Drupal/Tests/Core/Block/BlockManagerTest.php.

Here is a temporary patch for those that need it without the test. I'm sure it would be better updated as a merge request anyways.

Postponing this on #3050720: [Meta] Implement strict typing in existing code where (or in a sub-issue) we'll add deprecation support for adding return type hints.

Adding postponed item to the list of remaining tasks per the issue summary field documentation, remaining task.