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

Fix BaseBreadcrumbBlockService without extending MenuBlockService #687

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented Sep 1, 2022

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 jordisala1991 force-pushed the hotfix/fix-base-breadcrumb-block-service branch from 928524e to da9c3d5 Compare September 1, 2022 07:14
@jordisala1991
Copy link
Member Author

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

@jordisala1991 jordisala1991 force-pushed the hotfix/fix-base-breadcrumb-block-service branch 3 times, most recently from 2d3306f to ab3598d Compare September 4, 2022 17:14
@jordisala1991 jordisala1991 force-pushed the hotfix/fix-base-breadcrumb-block-service branch from ab3598d to 8418b44 Compare September 4, 2022 17:20
@jordisala1991 jordisala1991 marked this pull request as ready for review September 4, 2022 17:38
@jordisala1991
Copy link
Member Author

This is how it would be applied to SonataPageBundle: sonata-project/SonataPageBundle#1592

wdyt @VincentLanglet ?

VincentLanglet
VincentLanglet previously approved these changes Sep 4, 2022
@VincentLanglet
Copy link
Member

This is how it would be applied to SonataPageBundle: sonata-project/SonataPageBundle#1592

wdyt @VincentLanglet ?

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

@jordisala1991
Copy link
Member Author

I think we should be good now.

@jordisala1991
Copy link
Member Author

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

@jordisala1991 jordisala1991 merged commit f671f34 into sonata-project:3.x Sep 6, 2022
@jordisala1991 jordisala1991 deleted the hotfix/fix-base-breadcrumb-block-service branch September 6, 2022 07:14
@jordisala1991
Copy link
Member Author

Just tested: sonata-project/SonataPageBundle#1592

It works.

@core23
Copy link
Member

core23 commented Sep 12, 2022

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
Copy link
Member

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 84355f6

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

And an issue existed here #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
Copy link
Member

core23 commented Sep 12, 2022

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
Copy link
Member Author

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
Copy link
Member

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: #687 (comment)
But it was reported that a lot of things were broken #687 (comment).

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...

core23 added a commit to core23/SonataSeoBundle that referenced this pull request Sep 15, 2022
core23 added a commit to core23/SonataSeoBundle that referenced this pull request Sep 15, 2022
@core23 core23 mentioned this pull request Sep 15, 2022
core23 added a commit to core23/SonataSeoBundle that referenced this pull request Sep 15, 2022
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 this pull request may close these issues.

BaseBreadcrumbMenuBlockService changed from 2.x to 3.x
3 participants