Problem/Motivation
I am trying to make a single block which takes the place of core's page title & main content blocks on the block layout / region page. However, a minor core implementation detail is preventing me from doing so, and it has a simple fix.
`block/src/Plugin/DisplayVariant/BlockPageVariant.php` populates the Page Title and Main Content blocks when they are placed via Block Layout. It determines whether the block should receive the page title or main content based on whether the block implements the appropriate interface (MainContentBlockPluginInterface / TitleBlockPluginInterface).
Classes can implement many interfaces. I would like to create a single block which implements both interfaces and, therefore, can fulfill the role of both the main content block and the page title block.
Today, this cannot be done because core's code assumes a block will only implement at most one of the interfaces, as shown here:
if ($block_plugin instanceof MainContentBlockPluginInterface) {
$block_plugin->setMainContent($this->mainContent);
$main_content_block_displayed = TRUE;
}
elseif ($block_plugin instanceof TitleBlockPluginInterface) {
$block_plugin->setTitle($this->title);
}
elseif ($block_plugin instanceof MessagesBlockPluginInterface) {
$messages_block_displayed = TRUE;
}
Core assumes a block will implement only one of MainContentBlockPluginInterface, TitleBlockPluginInterface, or MessagesBlockPluginInterface. A custom/contrib block should be allowed to implement all 3.
Steps to reproduce
Create a custom block which implements MainContentBlockPluginInterface & TitleBlockPluginInterface. Drupal will not call $block_plugin->setTitle.
While reproducing the problem is not straightforward (requires a custom module), I hope the behavior can be seen in the code above.
Proposed resolution
Change both occurrences of `elseif` to `if` in the code sample above.
Remaining tasks
I'll submit a merge request shortly.
User interface changes
None. This won't impact core and is very unlikely to affect contrib - it simply unlocks an additional capability.
Release notes snippet
Previously, Drupal core assumed that blocks would implement at most one of MainContentBlockPluginInterface, TitleBlockPluginInterface, or MessagesBlockPluginInterface. This is no longer the case. The implication is that it is now possible to programmatically create a custom block which implements one or more of those interfaces. This change will have no impact to existing or new sites.
| Comment | File | Size | Author |
|---|
Issue fork drupal-3561564
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3561564-support-the-same
changes, plain diff MR !14039
Comments
Comment #2
ashrafabedI've submitted a merge request which addresses this issue.
I understand the impact isn't large, but I hope the explanation makes sense and the minimalistic nature of the changes make it possible to get this merged in.
Thank you! Please feel free to message me on slack if you would like to chat about it in realtime.
Comment #3
cilefen commentedThere is a branch but apparently no merge request. Also, all merge requests for Drupal Core must be targeted to the 11.x branch at this time.
Comment #5
ashrafabedThank you very much @cilefen.
I submitted the merge request to 11.x: https://git.drupalcode.org/project/drupal/-/merge_requests/14039
Comment #6
ashrafabedComment #7
smustgrave commentedTagging for framework manager but to me this seems like a feature that belongs in a contrib module. Don't think it hits 80% of site use cases.
Comment #8
smustgrave commentedActually may not need framework manager sign off. May need test coverage though will try and find out
Comment #9
pameeela commented@smustgrave asked about this in Slack. I definitely agree it's an edge case but considering it's a very simple change that shouldn't interfere with the main use case, it seems fine to accommodate it. (Should it prove not to be simple for _reasons_ then all bets are off!)
Comment #10
ashrafabedI'm glad to hear that - thank you both for taking the time to discuss it! I tagged it as a feature request but feels more like removing a restriction which _may_ have been unintentional in the first place. Hopefully it can get reviewed and merged in.
Comment #11
smustgrave commentedThink we will need test coverage for the change
Comment #12
ashrafabedI just pushed a commit which contains a test that fails before the change, but passes after it.
Let me know if this is adequate for the change. Thank you!
Comment #13
ashrafabedComment #14
smustgrave commentedThanks! Went to review but see it's got phpcs and phpstan issues if you mind addressing!
If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
Comment #15
ashrafabedThanks for the quick follow up! I pushed another commit which should address the coding standards issues.
Comment #16
smustgrave commentedNew functions in core/modules/block/tests/modules/block_test/src/Plugin/Block/TestPageTitleMainContentAndMessagesBlock.php will need return types to pass phpstan
Thanks!
Comment #17
ashrafabedThere are no new functions in core/modules/block/tests/modules/block_test/src/Plugin/Block/TestPageTitleMainContentAndMessagesBlock.php
- setMainContent is from MainContentBlockPluginInterface
- setTitle is from TitleBlockPluginInterface
- build is from BlockPluginInterface
Adding return types would cause a mismatch/incompatibility with the corresponding interface.
Let me know if I'm missing something! Thanks again for reviewing.
Comment #18
smustgrave commentedYou can still add them and it shouldn’t cause an issue even if the parent doesn’t. Else the baseline needs to be updated to include these
Comment #19
ashrafabedYou are correct, I learned something new: https://www.php.net/manual/en/language.oop5.variance.php
It looks like phpstan now passed in pipelines.
I do see an unrelated failure:
Because that failure seems unrelated and the tests pass for me locally, I'm marking it needs review.
Thanks again for your time.
Comment #20
ashrafabedComment #21
ashrafabedRebased to the latest 11.x and updated tests -- pipelines passed!
Please let me know if anything else is needed for this to be merged.
Comment #22
ashrafabedComment #23
smustgrave commentedHey super close! I think the test actually can be simplified and moved into BlockTest.php which extends BlockTestBase and already installs the modules you need. Then in your test you can call the placeBlock (move from the setup in your test) and should be good.
Comment #24
ashrafabedDone & done!
Comment #25
smustgrave commentedThink this is good to go
Comment #27
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #28
smustgrave commentedBot rebellion
Comment #29
longwaveMR needs rebasing against main.
Overall this change is small and I think it's a good idea, there is no reason why there can't be a single implementation of these multiple interfaces.
Comment #30
smustgrave commentedRebased through the UI, luckily it worked.
Comment #33
longwaveFeature requests are only eligible for a minor release, although I guess this wouldn't really hurt anything to go into a patch release. But unless anyone feels strongly, this change will ship with 11.4.0.
Committed and pushed 8ada62c8b16 to main and a3bde8dfdf8 to 11.x. Thanks!