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

Create new Breadcrumb interface #398

Merged
merged 1 commit into from
Sep 4, 2020

Conversation

core23
Copy link
Member

@core23 core23 commented Jun 6, 2020

Subject

There is no commen interface for breadcrumbs yet. A breadcrumb must implement a handleContext method.

I am targeting this branch, because this feature is BC.

Changelog

### Added
- Added new `BreadcrumbInterface`

@core23 core23 added the minor label Jun 6, 2020
@core23 core23 requested a review from a team June 6, 2020 19:52

use Sonata\BlockBundle\Block\Service\BlockServiceInterface;

interface Breadcrumb extends BlockServiceInterface
Copy link
Member Author

Choose a reason for hiding this comment

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

We must extend the BlockServiceInterface, because we must pass a valid block service here:
https://github.com/sonata-project/SonataSeoBundle/pull/398/files#diff-eb4c49edd518cf23ec8cccf9141b23f1R69

Copy link
Member

@wbloszyk wbloszyk left a comment

Choose a reason for hiding this comment

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

WDYT about create breadcrumb Block without interface? We can inject menu and menu leaf. Parents road will be new breadcrumb menu. Core menu will work as roadmap.

@core23
Copy link
Member Author

core23 commented Jun 7, 2020

WDYT about create breadcrumb Block without interface?

If I got you right, you prefer an abstract class over an interface? What's the benefit?

We got the actual problem that we overused inheritance for breadcrumbs.

@wbloszyk
Copy link
Member

wbloszyk commented Jun 7, 2020

I prefer using one final class with advance configuration. User should inject menu builded on knp-menu. Current uri will tell wich leaf id used. Taking parents we can build breadcrumb.

In PageBundle we can go Evans farder. Create sidemap menu and inject it to this Block.

@core23
Copy link
Member Author

core23 commented Jun 8, 2020

I prefer using one final class with advance configuration. User should inject menu builded on knp-menu. Current uri will tell wich leaf id used. Taking parents we can build breadcrumb.

Can you give an example how this will work with contexts?

At the moment we have many breadcrumbs that are registered. We use the sonata_block_render_event twig function and pass the context to it. Then the listener will decide which breadcrumb is rendered.

@core23
Copy link
Member Author

core23 commented Jul 10, 2020

Please review @sonata-project/contributors

VincentLanglet
VincentLanglet previously approved these changes Jul 10, 2020
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

Breadcrumb lr breadcrumb interface depending on the sonata standard ?

@core23
Copy link
Member Author

core23 commented Jul 10, 2020

Breadcrumb lr breadcrumb interface depending on the sonata standard ?

Do you mean the Interface suffix? AFAIK we have no communicated standard for this.

@VincentLanglet
Copy link
Member

Breadcrumb lr breadcrumb interface depending on the sonata standard ?

Do you mean the Interface suffix? AFAIK we have no communicated standard for this.

Yes, there was already a discussion about this and some classes have the suffix but some others doesn’t. It’s a symfony standard but not a communicated sonata one.

That’s why I approved the PR

@core23 core23 requested a review from a team July 10, 2020 12:09
@core23
Copy link
Member Author

core23 commented Jul 10, 2020

Maybe some other @sonata-project/contributors can give his opinion if we should add technical prefixes / suffixes

@greg0ire
Copy link
Contributor

greg0ire commented Jul 12, 2020

I find the suffix bad , but I think we mostly use it in Sonata, so to me, the real question is : are we ok with having both? Cause renaming all the existing interface in a BC way would be quite hard I think.

@VincentLanglet
Copy link
Member

I find the suffix bad , but I think we mostly use it in Sonata, so to me, the real question is : are we ok with having both? Cause renaming all the existing interface in a BC way would be quite hard I think.

I do like the suffix, and it's commonly used in Symfony. But it's a personal/team choice.

All interface of SonataAdmin have the suffix. I would prefer to be consistent.

@core23
Copy link
Member Author

core23 commented Jul 12, 2020

All interface of SonataAdmin have the suffix. I would prefer to be consistent.

That my be right, but we already support both naming schemas. E.g. this one introduced interfaces without the suffix: sonata-project/SonataFormatterBundle#417

@VincentLanglet
Copy link
Member

That my be right, but we already support both naming schemas. E.g. this one introduced interfaces without the suffix: sonata-project/SonataFormatterBundle#417

Yes. Maybe we should decide now if we want to keep supporting both naming.
Or we want to only use one (and then deprecate/renaming in next major the others).

The sooner we decide the less work we will have. @sonata-project/contributors

@greg0ire
Copy link
Contributor

greg0ire commented Jul 12, 2020

I'd open an issue on dev-kit and link to previous similar discussions in it.

@core23
Copy link
Member Author

core23 commented Sep 4, 2020

I added the Interface suffix, because it's symfony coding standard: https://symfony.com/doc/master/contributing/code/standards.html

@core23 core23 requested a review from wbloszyk September 4, 2020 13:58
@VincentLanglet VincentLanglet merged commit 8831441 into sonata-project:2.x Sep 4, 2020
@VincentLanglet
Copy link
Member

Thanks @core23

@core23 core23 deleted the breadcrumb-interface branch September 4, 2020 14:37
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.

None yet

4 participants