Problem/Motivation

If the SystemMainBlock is used by a module like Page Manager, it will cause a fatal error.
See #2819219: Fatal error when "setMainContent()" method is not called (block module not installed)

Proposed resolution

Restrict the block to block.module.

Is this an abuse of the context system?

Remaining tasks

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
2.77 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2825497-block-2.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.02 KB
2.31 KB

Just testing things out.

Status: Needs review » Needs work

The last submitted patch, 4: 2825497-block-4.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
5.09 KB

Okay maybe this isn't a good idea.

Status: Needs review » Needs work

The last submitted patch, 6: 2825497-block-6.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.08 KB

This will still fail, but rerolling just the same

Status: Needs review » Needs work

The last submitted patch, 9: 2825497-block-9.patch, failed testing.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jiv_e’s picture

I'm sorry to interrupt, but why is this a better solution than returning an empty array by default? Is there some other problem you are solving?

This seems overly complex to me. The fatal error is avoided by returning an empty array, right?

tim.plunkett’s picture

If the block is in the situation where it would return an empty array, it should never have been available to use in the first place.

jiv_e’s picture

I think the idea of restricting the main block to block module is better than just throwing a fatal error for other modules. But this is still looking overly complex to me. I have argumented for the benefits of the simpler solution here: https://www.drupal.org/node/2819219#comment-12135122

I have not yet seen any practical reason for "it should never". Can you help me out to understand those? Also could there be some case where other modules could need an access to the main block? Thanks!

jiv_e’s picture

Hi!

I'm wondering why my questions go unanswered. Are they inappropriate? These are honest questions so if there's something wrong how I present them I would be happy to be guided!

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

EclipseGc’s picture

So, full disclosure: I support the spirit, if not the specific implementation, of what Tim's suggesting here.

@jiv_e

I think your points, on this issue and the other you've referenced, are fine and appropriate. That being said, I'd start my counter argument by pointing out that contextual plugins (of which blocks are a member) work a little differently that perhaps what you're expecting.

Contextual plugins operate on the idea that there are certain method which cannot be invoked until context is set on the plugin. Other methods (like forms for configuration) CAN be invoked w/o context because it's not necessary for them. The block build() method is one of these methods that requires contexts to already be set. The conversion of the MainContent Block is one of those things I started and with help from others, we ultimately got working with the new plugin system. That being said, "context" came along a little later, and the main content block was never revisited subsequent to the introduction of that api addition. Which is to say that the setter that plugin uses is weird, non-standard, and there's already a standard for plugin which need that sort of handling... it's called context(s).

To address Tim's solution more directly, I'd actually invent a new type of context object that only the Main content block requires. That would give us an object on which we could place the expected render array. If the context isn't provided via a typical context provider, then no one will ever accidentally get it, and the block system itself could just hardcode the service id of that context. This might actually make the render pipe-line smoother and clearer as there'd be a service to which objects could read/write main content output. That's just my 2 cents on the approach. I'd love to iterate on this more and see where it goes.

Eclipse