sonata-project / SonataSeoBundle

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

BaseBreadcrumbMenuBlockService should not extends MenuBlockService #443

Closed VincentLanglet closed 3 years ago

VincentLanglet commented 3 years ago

Then we can make the Sonata\BlockBundle\Block\Service\MenuBlockService class final again.

https://github.com/sonata-project/SonataBlockBundle/pull/761

core23 commented 3 years ago

Maybe we should completely rethink this component. A breadcrumb should have nothing to do with a BlockService.

My idea is something like this:

namespace Sonata\SeoBundle\Breadcrumb;

interface Breadcrumb {
    public function configureSettings(OptionsResolver $resolver): void;

    public function getMenu(array $options = []): ItemInterface;
}
VincentLanglet commented 3 years ago

There is already a BreadcrumbInterface. https://github.com/sonata-project/SonataSeoBundle/blob/a3a55dcaea4d3acd9284e842a09fe4b566b94cda/src/BreadcrumbInterface.php

If we remove the extends MenuBlockService and add some methods, it could be enough.

I have some trouble to find why we extending MenuBlockService indeed. But I never used this bundle. Do you mind creating a PR/a draft @core23 ? :)

core23 commented 3 years ago

I have some trouble to find why we extending MenuBlockService indeed.

It has happend a long time ago when extend as many classes as possible was in vogue ;)

Do you mind creating a PR/a draft @core23 ? :)

Sadly I haven't much time for this huge refactoring. We also need some adjustments in the MenuBlock to get this thing working.

greg0ire commented 3 years ago

I have some trouble to find why we extending MenuBlockService indeed. But I never used this bundle.

I believe we did so to benefit from this piece of code: https://github.com/sonata-project/SonataSeoBundle/commit/84355f6ed55d7a3c95dcfdf2bbfbc1f6ee385eee#r42421762

wbloszyk commented 3 years ago

We can close it and fix BaseBreadcrumbMenuBlockService in #446