Non-cacheable block added to default handle makes every page non-cacheable

[vrann]

Preconditions

Magento on the latest develop branch, during the layout generation, collects all available layout handles for the particular page and merges instruction from them to the final layout structure of the page. Default layout handle is used as a basic for the every page. It makes the layout updates declared for the default handler appear on every page of the Magento.

Usually, layout update instructions look like referenceContainer or referenceBlock nodes with the layout content inside. It means "find the container (block) with the referenced name and use it as a parent of the content of the layout update". If layout update was added as a part of referenceContainer instruction to the default handler, there are good chances that the parent container is not a part of the particular page, because it was added using more exact layout handle. This leads to the situation, where some layout updates get merged and processed for the page but never executed. Parent container just does not exists on the page.

This is not the recommended practice to add layout updates to the default handler. A developer should rather find more exact layout handle which corresponds to the page where the block should actually appear. This saves resources on processing blocks which will never be rendered. However, this is an easy mistake to make, having that the layout update added to the default handle will actually execute on that particular page where it was intended to be executed. A developer might think that everything works good, despite the fact that resources are wasted.

While this is one of the generic issues with the layout declaration which while being annoying doesn't cause too many troubles (having that the layout declarations and data structures cached anyway), there is one more practical and more critical problem. If the block placed in the layout update has attributed declared cacheable=false it will effectively make any page where this block appears being non-cacheable. If it is added to the for the default handler it will make every page non-cacheable, even though this block is never executed.

The way how cacheable flag works is Layout collects all the layout updates for the certain page (merging updates for all the handles applicable to the page) and searches through them to find any block with the cacheable=false attribute declared. If there are such blocks found, it marks page as non-cacheable. Neither Full-Page Cache nor Varnish-adapter frameworks would attempt to cache such page.

Steps to reproduce

1. Create an extension. Add the layout update to the default handle via Vendor/Module/view/frontend/layout/default.xml
2. Reference some container which appears on the checkout page and add the layout update with the non-cacheable block

<referenceContainer name="order.success.additional.info">
    <block class="Vendor\Module\Block\SomeBlock" name="some_block" template="some.phtml" cacheable="false"/>
</referenceContainer>

3. Run the home page and check was it cached in full page cache; Run the order success page and check was it cached in the full page cache.

Expected result

1. Home page is cached; Order success page is not cached

Actual result

1. Neither home page nor order success page is not cached.

Notes

The reason why that happens is that block is added to the Scheduled Structure, to the list of blocks with the broken parents \Magento\Framework\View\Layout\ScheduledStructure::setElementToBrokenParentList. Such block will be removed from the page and will never be rendered.
At the same time, Full Page Cache is using the declaration of the layouts rather thanactually executed blocks structure, to find out whether the current page cacheable. This logic is encapsulated in the \Magento\Framework\View\Layout::isCacheable as an xpath expression over the layout updates structure for the current page.

[adragus-inviqa]

The problem here is that, by having a cacheable property on the block it makes people believe it's some sort of hole-punching. Whereas it's not: it's on the layout level.

I'd suggest taking this property out altogether and forget about it, or make it a property of the layout node. It hinders more than it helps.

[Fel1xx]

Additionally, even if you remove that non-cacheable block in your theme's layout file with something like <referenceBlock name="some_block" remove="true"/> the issue is still persisted. Only one way is to override layout file of that module.

[magento-engcom-team]

Hi @DariuszMaciejewski thank you for working on this. Closing the issue.

[mcjwsk]

@magento-engcom-team what's the reason of this decision?

[orlangur]

@DariuszMaciejewski probably it's just enough to get PR finished, no need in separate issue for tracking.