Skip to content

[Cms] Added a getByIdentifier() method to the BlockRepository #9163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

[Cms] Added a getByIdentifier() method to the BlockRepository #9163

wants to merge 4 commits into from

Conversation

enrico69
Copy link

@enrico69 enrico69 commented Apr 7, 2017

No description provided.

@adragus-inviqa
Copy link
Contributor

So you never tried loading a block via its identifier? Try it. It already works.

* @return \Magento\Cms\Api\Data\BlockInterface
* @throws \Magento\Framework\Exception\NoSuchEntityException
*/
public function getByIdentifier($blockIdentifier, $storeId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, due to backwards compatibility policy, currently we can not add any new methods to the interfaces, specially the ones marked with the @api annotation. You may extract this method to a separate interface/class, if it is really needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you suggest? Should I simply remove this part from the interface and let the implementation in the model ( app/code/Magento/Cms/Model/ResourceModel/Block.php ) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, your PR is redundant, as Magento\Cms\Api\BlockRepositoryInterface::getById('my-block-identifier') already works. As is Magento\Cms\Api\BlockRepositoryInterface::getById(12), too.

The resource model I showed you is just the place where the logic decides which route to go: identifier or numeric id? Again, try it yourself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but there is also this point, given by the user convenient

Might be useful to be aware of #6570 and #7417 in which identifier is not enough to update cms blocks by identifier when that identifier is shared across multiple stores.

…in order to avoid to break backward compatibility of the API
Copy link
Author

@enrico69 enrico69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@enrico69
Copy link
Author

@ishakhsuvarov changes are completed.

@okorshenko
Copy link
Contributor

okorshenko commented Jun 14, 2017

Hi @enrico69
Thank you for your contribution. Unfortunately, we can not accept this PR. I agree with the point of @adragus-inviqa . Looks like this will be duplication of the functionality.
Thank you for your efforts.

Just FYI: we are working on #7417 It should resolve your issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants