Full Page Cache Logic in Catalog Module

[sshymko]

Classes Magento\Catalog\Plugin\Model\Indexer\Category\Product\Execute and Magento\Catalog\Plugin\Model\Resource\Attribute\Save implement flushing the Full Page Cache. They should belong to the Magento_PageCache module, not the Magento_Catalog. Utilization of the constant Magento\PageCache\Model\Cache\Type::TYPE_IDENTIFIER rather than literal full_page would expose that dependency on the code level.

[orlangur]

They should belong to the PageCache module, not the Catalog

This suggestion is wrong. PageCache module composed in such a way would contradict to "true modularity": the intention to have granular and loosely coupled system components.

PageCache module in Magento 2 introduces common mechanism for any other module to implement page caching. It must not contain any domain-specific logic.

Other modules, like Catalog, may involve this mechanism and implement their own page caching domain-specific logic. Thus Catalog module should have soft dependency on PageCache meaning that caching ability is optional: if PageCache module is present it works and if no nothing bad happens.

Surely, it is possible to go even further and implement such caching as separate module like CatalogPageCache. Or collect caching for a bunch of modules in one with no other module depending on this new module. But, again, such code must not be placed in PageCache module itself.

[sshymko]

Correct, if a dependency of Catalog on PageCache is permitted, than implementation is correct except for implicit reference to the Full Page Cache constant. Good job on breaking down the monolithic FPC module, didn't notice it at first!

[sshymko]

Still, constant Magento\PageCache\Model\Cache\Type::TYPE_IDENTIFIER needs to be used to make dependency explicit.

[orlangur]

No objections on this from my side. Catalog already depends on PageCache due to classes used anyway.

This dependency is logically soft but declared as hard because there is no way to tell "I use classes of some module in plugins only" in dependency declaration currently.

[verklov]

MAGETWO-33021

[piotrekkaminski]

Thank you for your submission.

We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues.

Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here).

We are closing this GitHub ticket and have moved your request to the new forum.