sonata-project / SonataSeoBundle

Symfony SonataSeoBundle
https://docs.sonata-project.org/projects/SonataSeoBundle
MIT License
137 stars 90 forks source link

Fix BaseBreadcrumbBlockService without extending MenuBlockService #687

Closed jordisala1991 closed 2 years ago

jordisala1991 commented 2 years ago

Subject

I am targeting this branch, because this is broken since 3.0, and even if it introduces BC breaks, it does because currently the block is fundamentally broken in the context of SonataPageBundle.

Closes #685.

Changelog

### Fixed
- [BC break] Breadcrumbs now work in the context of SonataPageBundle blocks, broken since 3.0.
jordisala1991 commented 2 years ago

Have to fix tests, but this is what I had in mind @VincentLanglet

jordisala1991 commented 2 years ago

This is how it would be applied to SonataPageBundle: https://github.com/sonata-project/SonataPageBundle/pull/1592

wdyt @VincentLanglet ?

VincentLanglet commented 2 years ago

This is how it would be applied to SonataPageBundle: https://github.com/sonata-project/SonataPageBundle/pull/1592

wdyt @VincentLanglet ?

Might be worth to add a note in the upgrade note, just in case

jordisala1991 commented 2 years ago

I think we should be good now.

jordisala1991 commented 2 years ago

Im merging, but do not release yet, I want to validate on SonataPageBundle (with Github Actions) that it works as expected.

jordisala1991 commented 2 years ago

Just tested: https://github.com/sonata-project/SonataPageBundle/pull/1592

It works.

core23 commented 2 years ago

So you just introduced a BC break for any other project that is relying on the BaseBreadcrumbBlockService. Great work...

Why didn't you fix the problem on the SonataPageBundle

VincentLanglet commented 2 years ago

So you just introduced a BC break for any other project that is relying on the BaseBreadcrumbBlockService. Great work... Why didn't you fix the problem on the SonataPageBundle

Seems like he had to fix the great work of someone else https://github.com/sonata-project/SonataSeoBundle/commit/84355f6ed55d7a3c95dcfdf2bbfbc1f6ee385eee

The missing Editable implementation was already reported here: https://github.com/sonata-project/SonataSeoBundle/commit/84355f6ed55d7a3c95dcfdf2bbfbc1f6ee385eee#r42421991

And an issue existed here https://github.com/sonata-project/SonataSeoBundle/issues/685 explaining that the block service wasn't working/fully working. To us, it was not a PageBundle issue but a SeoBundle one and we were considering this as a bug fix ; but every change break someone workflow (https://xkcd.com/1172/). The issue was here to try to get some opinions but you didn't seemed concerned about this issue before (or any other recent sonata issues/pr).

Jordisala1991 is currently doing an excellent and time-consuming work for PageBundle/BlockBundle and in more general way sonata-project. Like always, better complaining than helping.

core23 commented 2 years ago

The issue was here to try to get some opinions but you didn't seemed concerned about this issue before (or any other recent sonata issues/pr).

Jordisala1991 is currently doing an excellent and time-consuming work for PageBundle/BlockBundle and in more general way sonata-project. Like always, better complaining than helping.

First of all cudos to both of you by keeping the project alive ❤️

Also I'm not actively maintaining the sonata projects anymore for many different reasons, I was also contributing to this projects for many years and hundreds of hours and always tried to stay BC on the stable where it was possible. So you should only change method signatures in a major release (for non-final classes).

The missing Editable implementation was already reported here: 84355f6#r42421991

Also if there might be a bug in this component, the issue could also be fixed with respect to the BC policy we have for many years by writing a deprecation layer for this. This could be done by making the constructor arguments nullable and throwing a deprecation warning or by creating a PageBundle specific implementation.

I was using the old implantation since the 3.0 release in many of my projects without any problem.

Some side information why I removed the editable part: The breadcrumb should be useable without any frontend editing component (e.g. PageBundle). You should also be able to setup all options manually or via hardcoded configurations. If you want the editing stuff and need form editing stuff, you should either introduce an EditableBaseBreadcrumb abstract class or this to the specific breadcrumb class that should be editable.

jordisala1991 commented 2 years ago

Hey! If you have a better solution, feel free to provide all the PRs needed on all the project to make it work. I mean, on the SonataSeoBundle, and SonataPageBundle.

I am sorry if this caused you trouble on your projects, but it is not my intention to change this code again. Thank you for understanding.

VincentLanglet commented 2 years ago

The missing Editable implementation was already reported here: 84355f6#r42421991

Also if there might be a bug in this component, the issue could also be fixed with respect to the BC policy we have for many years by writing a deprecation layer for this. This could be done by making the constructor arguments nullable and throwing a deprecation warning or by creating a PageBundle specific implementation.

I first proposed this here: https://github.com/sonata-project/SonataSeoBundle/pull/687#discussion_r960305607 But it was reported that a lot of things were broken https://github.com/sonata-project/SonataSeoBundle/pull/687#discussion_r960443979.

I was using the old implantation since the 3.0 release in many of my projects without any problem.

If everything is broken, a BC-break is not really one since the class wasn't working on it own. Seems like it was usable finally ; I can't say how, since I don't know a lot of this stuff. Might be interesting to explain more about to @jordisala1991.

Some side information why I removed the editable part: The breadcrumb should be useable without any frontend editing component (e.g. PageBundle). You should also be able to setup all options manually or via hardcoded configurations. If you want the editing stuff and need form editing stuff, you should either introduce an EditableBaseBreadcrumb abstract class or this to the specific breadcrumb class that should be editable.

This might be a better idea but now the question would be: should we keep the BC break now it's done or should we fix it differently, but reverting the BC break will be also a BC break...