Introduce separate BlockByIdentifier class to get Layout Block based on CMS Block Identifier

[lbajsarowicz]

Description (*)

The existing class is anti-pattern:

1. Uses \Magento\Framework\Model\AbstractModel::load to load Model
2. If block_id is identifier, the getIdentities method returns same identity whatever the Scope is (return [\Magento\Cms\Model\Block::CACHE_TAG . '_' . $this->getBlockId()];), which is painful when you use the same identifier for multiple per-store CMS blocks.

Why can't we fix the original Block? Some of developers might have used:

        <referenceContainer name="page.top">
            <block class="Magento\Cms\Block\Block" name="header.info" after="navigation.sections">
                <arguments>
                    <argument name="block_id" xsi:type="string">666</argument>
                </arguments>
            </block>
        </referenceContainer>

That is why we keep old Block backwards-compatible and introducing new one that can be used explicitly:

        <referenceContainer name="page.top">
            <block class="Magento\Cms\Block\BlockByIdentifier" name="header.info" after="navigation.sections">
                <arguments>
                    <argument name="identifier" xsi:type="string">header-info</argument>
                </arguments>
            </block>
        </referenceContainer>

Related Pull Requests

Fixed Issues (if relevant)

1. magento/magento2#<issue_number>: Issue title

Manual testing scenarios (*)

1. ...
2. ...

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

Resolved issues:

1. resolves [Issue] Introduce separate BlockByIdentifier class to get Layout Block based on CMS Block Identifier #28309: Introduce separate BlockByIdentifier class to get Layout Block based on CMS Block Identifier

[m2-assistant]

Hi @lbajsarowicz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[lbajsarowicz]

After further investigation, I realized that additional Identities are not necessary, as these are used only as tags, not as cache keys. The only thing that was missing before and is missing now is to tag block with CMS Block identities, so that when CMS Block is requested to be removed from cache, the Layout Block cache should be cleaned too.

I'm going to adjust that now.

[ihor-sviziev]

Hi @lbajsarowicz,
Could you fix unit tests failures ?

[lbajsarowicz]

I can't find any failing report for Static Tests, neither for Unit Tests.

[ihor-sviziev]

Hi @slavvka could you add more info why unit and static tests fails? We don’t have anything in reports

[ihor-sviziev]

@magento run all tests

[magento-engcom-team]

Hi @sidolov, thank you for the review.
ENGCOM-7560 has been created to process this Pull Request

[vovayatsyuk]

What issues?

[vovayatsyuk]

Must be this one, that was fixed? 5c84c1a

[lbajsarowicz]

Not really. Except the fact the previous one uses both id and identifier for cache key, there was something more (I'll try to find out what was the other issue)

[vovayatsyuk]

I don't understand why do Magento needs this new block. It seems that everything from this PR could be done in the existing block without creating a new entity and without BC.

Something like this:

if (is_numeric($identifier)) {
  // loadById to be compatible with custom code that uses id.
} else {
 // new logic
}

[lbajsarowicz]

@vovayatsyuk If we want to follow SOLID principles - the arguments have to explicit, without type juggling. It's exactly the same like Page by Identifier interface.

[vovayatsyuk]

But we already have a block to show cms blocks. I can only agree with this change if we'll remove another block in the future. But this leads to the next thought - why create a new block instead of refactoring the existing one and remove deprecated logic in the next major release?

[lbajsarowicz]

@vovayatsyuk The only issue for introducing that Interface was the requirement for backwards compatibility, as this PR was created before 2.4 was released. If I knew that's going to hang for a year, I'd just modify the original implementation with happy "do it all" (against the SOLID principles) way.

[ihor-sviziev]

@vovayatsyuk in my opinion - we can go with both options. in the current implementation of cms/block/block we have following issues:

1. not enforced to use CMS block ID or Identified, and both currently supported and widely used by extension developers
2. different cache tags in case if block ID or identified was used
3. deprecated method of loading the block (using $model->load())
4. In case if the block with such identifier / id is missing / disabled / not assigned to current store view - we don't have any errors/warnings/etc.

Also as @lbajsarowicz said - addressing all of these issues will cause the not following SOLID principles and probably will be backward incompatible.
Technically this block isn't marked as @api and we might add backward incompatible changes, but if we'll change the logic - it definitely will cause issues during magento upgrade.

[ihor-sviziev]

From the discussion above - seems like we all agreed that this solution is good enough to be accepted.
@magento run all tests

[ihor-sviziev]

@sidolov is there any reason why this PR isn't getting merged?

[sidolov]

@ihor-sviziev the pull request in the delivery queue right now and will be delivered soon. There are no reasons to postpone the merging process.

[m2-assistant]

Hi @lbajsarowicz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[lbajsarowicz]

@sidolov , @ihor-sviziev @vovayatsyuk - Thanks for your support. Glad to have this merged.