By specification of \Drupal\Core\Block\BlockPluginInterface, the build() method must return an array, but in case of \Drupal\system\Plugin\Block\SystemMainBlock it could not always be respected. If method setMainContent() of \Drupal\system\Plugin\Block\SystemMainBlock class will not be called, then build() method of the same class will return null which may lead to TypeError.

  1. Install Drupal using minimal profile.
  2. Install the following modules: panels, page_manager, page_manager_ui.
  3. Create new custom page using "Page manager", select "Panels" as variant type.
  4. Add "Main page content" block to content of newly created page, visit it and you'll see The website encountered an unexpected error. Please try again later.
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

BR0kEN created an issue. See original summary.

BR0kEN’s picture

Status: Active » Needs review
FileSize
502 bytes
BR0kEN’s picture

Issue summary: View changes
BR0kEN’s picture

I'm not sure that in body of issue described correct workflow, because code of panels module have the following lines:

          $content = $block->build();
          if (Element::isEmpty($content)) {

But what I know exactly - is that we should ensure that return type will be an array.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Looks good for me

lauriii’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 24eeeb1 to 8.3.x and ba6f496 to 8.2.x. Thanks!

I thought about asking for a test but it just does not seem worth it.

  • alexpott committed 24eeeb1 on 8.3.x
    Issue #2819219 by BR0kEN: SystemMainBlock could lead to fatal error in...

  • alexpott committed ba6f496 on 8.2.x
    Issue #2819219 by BR0kEN: SystemMainBlock could lead to fatal error in...
Wim Leers’s picture

Status: Fixed » Active

If method setMainContent() of \Drupal\system\Plugin\Block\SystemMainBlock class will not be called

This is very much a non-option. There must be "main content" for a given page.

This resulting in an error/failing ungracefully was great. You would not be able to achieve that with Drupal core itself. If you could, that'd be a bug.

It sounds like something panels or page_manager are doing is wrong.

BR0kEN’s picture

@Wim Leers, let's abstract from Drupal core: could be a case when, instead of expected array, null will be returned. If it acceptable, then we need to update returned types in \Drupal\Core\Block\BlockPluginInterface::build().

Nevertheless I'm not thinking that this is mostly issue of Panels, because SystemMainBlock - is not a standard block - it implements additional MainContentBlockPluginInterface interface.

dawehner’s picture

There are some more possible options.

  • Fail silently
  • Fail loud, aka. throw an exception as something or log an error

But what we should NOT do is to return NULL, as that makes it harder to debug than needed.

BR0kEN’s picture

Status: Active » Fixed

@dawehner, I guess now we have first of the options you've described. Also, if page do not have a content, then it's not a requirement to call setMainContent().

I'm pretty sure that null-preventing behavior - is a good one.

@Wim Leers, I set back the Fixed status. Please create a new issue for enhancements.

Wim Leers’s picture

Status: Fixed » Active

No, I'm not asking for an enhancement. I'm saying this commit is wrong and should be reverted.

I also asked for detailed reasoning/explanation as to how SystemMainBlock can cause any problems when used with Panels/Page Manager, because with Drupal core alone you surely cannot get it to lead to a fatal error.

dawehner’s picture

Well, code should be correct for itself, outside of arbitrary side effects :)

  • xjm committed e6c5f46 on 8.2.x
    Revert "Issue #2819219 by BR0kEN: SystemMainBlock could lead to fatal...

  • xjm committed 148fb3f on 8.3.x
    Revert "Issue #2819219 by BR0kEN: SystemMainBlock could lead to fatal...
xjm’s picture

Title: SystemMainBlock could lead to fatal error in case if "setMainContent" method will not be called » Fatal error when "setMainContent()" method will not be called (block module not installed)
Project: Drupal core » Page Manager
Version: 8.3.x-dev » 8.x-1.x-dev
Component: system.module » Code

After discussing with @tim.plunkett and @wimleers, it sounds like this is actually a bug in Page Manager (we think). What core should do is throw an explicit exception like this:
https://www.pastery.net/zzwjnv/
(via @tim.plunkett)
Edit: and also document that it can be null.

So we'll open a separate core issue to do that in 8.3.x only (since it is a disruptive change), but in either case there is a bug that will need to be fixed in contrib.

dawehner’s picture

Is there a reason we cannot check whether main content !== NULL? The main content should be a render array, so checking for NULL should be enough.

xjm’s picture

Title: Fatal error when "setMainContent()" method will not be called (block module not installed) » Fatal error when "setMainContent()" method is not called (block module not installed)
BR0kEN’s picture

Could somebody give me a reason why value, equal to empty array by default, - bad approach?

We just will have nothing to render, when setMainContent() method will not be called. Easy enough.

tim.plunkett’s picture

If the system using the main content block does not call setMainContent, the block shouldn't be available to place in that system.

BR0kEN’s picture

So, do Page Manager needs to implement something like this?

      /** @var $blocks \Drupal\block\BlockInterface[] */
      foreach ($blocks as $key => $block) {
        $block_plugin = $block->getPlugin();
        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;
        }

Code above taken from \Drupal\block\Plugin\DisplayVariant\BlockPageVariant::build().

Do not it looks a bit harder than just render the emptiness? We can make a lot of specific blocks (via implementing the interfaces) and it's strange for me that we need writing a lot of conditions to handle specific initializations.

tim.plunkett’s picture

lauriii’s picture

According to #23 block plugins implementing TitleBlockPluginInterface or MessagesBlockPluginInterface are also not working properly with other modules than block module unless they specifically provide to support them.

Edit:
I read the code in BlockPageVariant::build and it loks like the MessagesBlockPluginInterface is used to provide fallback so that the messages are always printed on the page, even if there is no messages block placed. So the issue only exists for MainContentBlockPluginInterface and TitleBlockPluginInterface.

BR0kEN’s picture

#22 sounds good, but in real life all code that uses this block must have an additional check, like (taken from https://www.drupal.org/node/2812721#comment-11952030):

           $content = $block->build();

+          if (!is_array($content)) {
+            throw new \LogicException(sprintf(
+              '%s::build() method has to return an array. Class returning non-array value is: %s',
+              BlockPluginInterface::class,
+              get_class($block)
+            ));
           }
jiv_e’s picture

Ok.. this is confusing. @BR0kEN guided me here after I added this:
https://www.drupal.org/node/2885370

I don't understand the reason to allow NULL values from SystemMainBlock::build(). Documentation says it shouldn't. Is the documentation incorrect? I have read this thread and I still don't understand.

My patch passed the tests. What does it break? Not enough test coverage?

tim.plunkett’s picture

Issue tags: -TypeError

I'd prefer something along these lines.

diff --git a/core/modules/system/src/Plugin/Block/SystemMainBlock.php b/core/modules/system/src/Plugin/Block/SystemMainBlock.php
index beba38d..1984f21 100644
--- a/core/modules/system/src/Plugin/Block/SystemMainBlock.php
+++ b/core/modules/system/src/Plugin/Block/SystemMainBlock.php
@@ -33,6 +33,10 @@ public function setMainContent(array $main_content) {
    * {@inheritdoc}
    */
   public function build() {
+    if (!is_array($this->mainContent)) {
+      throw new \Exception('The system_main_block was placed but ::setMainContent was not called');
+    }
+
     return $this->mainContent;
   }
 
jiv_e’s picture

Thanks! I had fragile idea of this.. ok. It seemed obvious that the class was meant to be used like that.

But I don't agree that we should throw an Exception if mainContent is not set. Why should the system fail? Why not just let it go through as an empty array and use drupal_set_message and logger to notify that 'The system_main_block was placed but ::setMainContent was not called'?

jiv_e’s picture

I'll try to formulate two options I see presented in this thread. Then I will try to argue why the second option is better.

1. SystemMainBlock should be a special block. Instead like all the other blocks in core it's build() method should be called only after some requirements are met. Namely it's setMainContent() method should be called first. Otherwise SystemMainBlock::build() throws an Exception. It is contrib developers responsibility to handle this exception in her code. This special case and the Exception should be mentioned in the documentation of BlockPluginInterface::build().

2. SystemMainBlock behaves as all other blocks in core. It always returns an array. If setMainContent is not called then build() will return an empty array and SystemMainBlock shows no content. It's developers responsibility to call setMainContent if she wishes to have content on the SystemMainBlock.

Optionally we could notify users that setMainContent was not called, as mentioned in #29. But I changed my mind about that, so maybe not.

I think option 2. is better:

  1. It is simpler:
    1. Simpler code.
    2. Simpler for developers.
    3. Simpler for documentation.
    4. No need for additional tests for the Exception.
  2. It's less dangerous - removes one possibility of fatal error.
  3. There seems to be no harm to leave SystemMainBlock empty, except it will be empty. If it should have content, people will notice this and report a bug anyways. No need to fail hard by using Exception.

I will leave defending of the first option for others. Please correct me if I misrepresented the first option. If there's an option 3. add it in!

BR0kEN’s picture

To be honest, I've changed my mind about this block and now think that it should throw an exception because otherwise (in a case of emptiness) its usage seems redundant.

jiv_e’s picture

@BR0kEN, I agree that it seems redundant. But I think that the benefits of the option 2. still outweigh this.

I try to list all the reasons for option 1. I have noticed.

  1. There must be main content for a given page. / It's a bug.
  2. Without content the block seems redundant.

The first one has two forms, but they say the same thing. Empty main content block should not be accepted in any circumstance. The second one is just a weaker form of the first. It just says that it seems useless to have an empty main content block. So basically we should first think about the first one. If that does not hold then move to the next.

The problem with the first is that it doesn't really give any practical reasons. It just states that "you shouldn't do that". Please explain why! Explain also why these reasons outweigh the reasons for the 2. option.

For me it's expected to have an empty block if no content is put in it. If a developer/user wants to have content in it then she should put some in. It's not core's job to decide if blocks have content or not. So I would say it's not a bug.

Defender of option 1. is saying that the main content block should not exist on a page without a content. Is this just a philosophical standpoint without real practical reason? I think blocks are containers. Empty container should be able to exist without any content.

jiv_e’s picture

I'm sorry if my approach was inappropriate! I hope I didn't insult anyone. I admit I teased a little bit, though. ;)

I don't want to stall this issue, if I'm just raving please ignore me! I'm really just trying to understand the reasons behind option 1. and make sure that we make solid decisions for the benefit of Drupal.