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

Add menu uri voter #44

Merged
merged 1 commit into from
Oct 3, 2019
Merged

Add menu uri voter #44

merged 1 commit into from
Oct 3, 2019

Conversation

7ochem
Copy link
Contributor

@7ochem 7ochem commented Oct 1, 2019

For getting the current menu item, the Htmldev Bundle Styleguide relies on a service that is being added bij the Zicht Menu Bundle. Since the Zicht Menu Bundle is not a requirement, the Htmldev Bundle on a vanilla Symfony install does not have this service and cannot resolve the current menu item.

Since the Zicht Menu Bundle depends on Sonata, it would not be realistic to require the Zicht Menu Bundle within the Htmldev Bundle just to have this one single service available.

Therefor I added this service to the Htmldev Bundle.

@7ochem 7ochem merged commit c28790e into release/3.x Oct 3, 2019
@7ochem 7ochem deleted the fix/menu-uri-voter branch October 3, 2019 07:50
@boudewijn-zicht
Copy link
Contributor

@7ochem Does this mean that the service introduced here is also present in the MenuBundle? If so, that should be removed and this bundle should require at least the version of the MenuBundle where is no longer exists

@7ochem
Copy link
Contributor Author

7ochem commented Oct 7, 2019

Does this mean that the service introduced here is also present in the MenuBundle?

Yes. Both bundles have their own implementation

If so, that should be removed

It cannot be removed in the MenuBundle because the MenuBundle does not require the Htmldev bundle. Or do we want to force porjects into having the Htmldev bundle pulled in if they need the MenuBundle?

and this bundle should require at least the version of the MenuBundle where is no longer exists

This bundle requires only the knplabs/knp-menu-bundle. It does not need the zicht/menu-bundle for anything.

I think the solution I went for here (both bundles their own implementation) is the most clean solution for now.

@boudewijn-zicht
Copy link
Contributor

👍

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

Successfully merging this pull request may close these issues.

2 participants