Fatal error when "setMainContent()" method is not called (block module not installed)

Looks good for me

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.

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.

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.

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.

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

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.

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.

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

Opened #2825497: SystemMainBlock only functions when used by block.module.

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.

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?

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;
   }
 

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'?

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, 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.

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.