Skip to content
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

BaseBreadcrumbMenuBlockService changed from 2.x to 3.x #685

Closed
jordisala1991 opened this issue Aug 12, 2022 · 3 comments · Fixed by #687
Closed

BaseBreadcrumbMenuBlockService changed from 2.x to 3.x #685

jordisala1991 opened this issue Aug 12, 2022 · 3 comments · Fixed by #687
Labels

Comments

@jordisala1991
Copy link
Member

Environment

Sonata packages

Latest version 4, checking on SonataPageBundle

Symfony packages

Latest 6.1.x release

PHP version

All php versions supported (from 7.4 to 8.1)

Subject

Not sure how to fix it, or what is the best solution, but from 2.x to 3.x we removed EditableBlockInterface from BaseBreadcrumbMenuBlockService.

We stop extending this other class from SonataBlockBundle, but not sure why: https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Block/Service/MenuBlockService.php

Maybe it was to make that class final, but with that move we broke that block.

Possible solutions:

  1. Revert to extend MenuBlockService
  2. Implement EditableBlockService directly on BaseBreadcrumbMenuBlockService
  3. ... others?

Minimal repository with the bug

sonata-project/SonataPageBundle#1552

Steps to reproduce

Try to use https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Block/BreadcrumbBlockService.php block.

Expected results

Be able to use that SonataPageBundle block

Actual results

It will break because this class does not implements EditableBlockInterface

@VincentLanglet
Copy link
Member

Can't we extends BaseBreadcrumbMenuBlockService is PageBundle and implements EditableBlockService in PageBundle ?

@jordisala1991
Copy link
Member Author

Is that a better option than what was prior to the pr that broke the integration? Previously the block had those fields because it extended a class from block bundle that had it. Shouldnt the block provide everything needed to use it?

@VincentLanglet
Copy link
Member

Is that a better option than what was prior to the pr that broke the integration? Previously the block had those fields because it extended a class from block bundle that had it. Shouldnt the block provide everything needed to use it?

I wasn't sure the EditableBlock feature was needed for everyone extending this block or it was possible to create blocks extending this one which doesn't need the EditableBlock.

But yes, I'm not against implementing Editable block interface here. And it would be BC

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

Successfully merging a pull request may close this issue.

2 participants