Performance: Do not build blocks on Webform Share pages?

This behavior can be mitigated as easy as MR 279 shows.

Is changing the current behavior makes sense?

Wow, this MR is really clean. I am not sure we can add any additional test coverage.

Is there any reference or link to an example, you can provide for this pattern/solution?

BTW, I think we can merge this into 6.1.x

Created the MR to 6.1: https://git.drupalcode.org/project/webform/-/merge_requests/281

On MR 279 I just provided one improvement in the simple ! to empty(): https://git.drupalcode.org/project/webform/-/merge_requests/279/diffs?co...

@jrockowitz

Wow, this MR is really clean. /blockquote>

Thanks :)

I am not sure we can add any additional test coverage.

Well... if it worth the effort... maybe we can create a block that would throw an exception when it is rendered... add it to Webform share pages and see if it breaks the page... just an idea, I haven't tested it.

Is there any reference or link to an example, you can provide for this pattern/solution?

I do not have a reference, frankly, this API was also new to me :) No surprise, since only those modules were ever rely on it that manipulates display layouts... http://grep.xnddx.ru/search?text=PageVariantInterface&filename=
I have tried to figure out the magic behind Webform Share feature because I was also working on a generic solution for exposing the main content region of ANY Drupal page in an iframe friendly way, this is how I found this API, called by \Drupal\Core\Render\MainContent\HtmlRenderer::prepare() indirectly (via fireing a \Drupal\Core\Render\PageDisplayVariantSelectionEvent event).

We don't need test coverage.

I tweaked the code a little to align with webform code conventions and removed the old block handling code.

https://git.drupalcode.org/issue/webform-3336876/-/commit/f44df2945cadb3...

I think this will have to be an improvement for 6.2.x only.

I tweaked the code a little to align with webform code conventions

It is your final call, but I would not open this class for inheritance and make it part of the public API of the module. This is just an event subscriber that takes care of the job. IMO, Nobody should consider this as an API. If they have a good reason to extend it somewhere, use the decorator pattern (composition) instead of inheritance.

I think this will have to be an improvement for 6.2.x only.

May I ask why did you change your "BTW, I think we can merge this into 6.1.x" since #4? :)

Why 6.2.x?

People might be relying on or altering how the main content is rendered, which will change very slightly via this patch. For example, I was previously manually adding the title block.

Why not use 'final' via classes?

No classes in the Webform module are currently using 'final.' It might be a good idea to finalize some classes, but it should be done in a dedicated ticket with references to Drupal core's policy around using 'final'.

Ack 🙂

Automatically closed - issue fixed for 2 weeks with no activity.