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

version 3 (compatible with KnpMenu v3) #412

Open
wants to merge 11 commits into
base: master
from

Conversation

@garak
Copy link
Collaborator

commented Sep 6, 2019

No description provided.

Copy link
Collaborator

left a comment

Compiler passes should also become final.

CHANGELOG.md Outdated Show resolved Hide resolved
// Remove the old way of handling this feature.
$container->removeDefinition('knp_menu.menu_provider.container_aware');
$container->removeDefinition('knp_menu.menu_provider.builder_service');

This comment has been minimized.

Copy link
@stof

stof Sep 6, 2019

Collaborator

you should drop this service definition entirely now, as it is always useless when the LazyProvider is used.

CHANGELOG.md Show resolved Hide resolved
src/Resources/config/menu.xml Outdated Show resolved Hide resolved
$definition->replaceArgument(2, $renderers);
}
$locator = ServiceLocatorTagPass::register($container, $rendererReferences);
// Replace the service definition with a PsrProvider

This comment has been minimized.

Copy link
@stof

stof Sep 6, 2019

Collaborator

the replacement itself should be dropped. The service should be a PsrProvider directly in the XML config file now, and we should only replace the first argument with the locator here.

This comment has been minimized.

Copy link
@garak

garak Sep 20, 2019

Author Collaborator

@stof in a previous comment you said that definition of Knp\Menu\Provider\PsrProvider service should be dropped (since it's removed using LazyProvider), now you say that the service should be in XML config. Can you clarify?

This comment has been minimized.

Copy link
@stof

stof Sep 20, 2019

Collaborator

you are confusing things. This compiler pas is about the renderer provider, not the menu provider.

The PsrProvider for renderer provider must be used, as that's what get used for Symfony 3.3+. But the service definition using the PsrProvider should be in the XML file directly, instead of defining the service for the old ContainerAwareProvider in XML and replacing it entirely here.
Then, this compiler pass should be only replacing the argument to inject the service locator.

This comment has been minimized.

Copy link
@garak

garak Sep 20, 2019

Author Collaborator

So, should we remove ContainerAwareProvider class too?

@@ -23,7 +23,6 @@ public function process(ContainerBuilder $container)
}
$definition = $container->getDefinition('knp_menu.matcher');
$listener = $container->getDefinition('knp_menu.listener.voters');
$hasRequestAwareVoter = false;

This comment has been minimized.

Copy link
@stof

stof Sep 6, 2019

Collaborator

should be dropped now (as well as the following code using it)

This comment has been minimized.

Copy link
@stof

stof Sep 20, 2019

Collaborator

this still need to be dropped, as this variable is not used anymore.

src/Resources/config/menu.xml Outdated Show resolved Hide resolved
@stof stof referenced this pull request Sep 6, 2019
@garak garak force-pushed the garak:v3 branch from af5531d to 5f20781 Sep 18, 2019
garak added 7 commits Sep 6, 2019
@garak garak force-pushed the garak:v3 branch from 3cbd369 to 4421837 Sep 19, 2019
garak added 2 commits Sep 20, 2019
@@ -12,9 +12,9 @@
*
* @internal
*/
class MenuBuilderPass implements CompilerPassInterface
final class MenuBuilderPass implements CompilerPassInterface

This comment has been minimized.

Copy link
@stof

stof Sep 20, 2019

Collaborator

please tag them as @final in a 2.x release to be safe.

This comment has been minimized.

Copy link
@phansys

phansys Sep 27, 2019

Which branch is intended to target 2.x release? Currently, there are only 2: v3 and master.

This comment has been minimized.

Copy link
@garak

garak Sep 28, 2019

Author Collaborator

@phansys master

This comment has been minimized.

Copy link
@phansys

phansys Sep 28, 2019

Then, shouldn't this PR target v3?

This comment has been minimized.

Copy link
@garak

garak Sep 28, 2019

Author Collaborator

No, v3 is just a temporary branch where this PR is already merged, useful to do a practical test.

<argument type="service" id="service_container" />
<argument type="collection" />
</service>

<service id="knp_menu.menu_provider.builder_service" class="Knp\Bundle\MenuBundle\Provider\BuilderServiceProvider" public="false">

This comment has been minimized.

Copy link
@stof

stof Sep 20, 2019

Collaborator

this service must be removed. It is useless now that the lazy provider can always be used (see the compiler pass always removing it). And the class should be removed too.

@@ -72,8 +70,7 @@ public function testDisableContainerAwareProvider()
{
$container = new ContainerBuilder();
$loader = new KnpMenuExtension();
$loader->load([['providers' => ['container_aware' => false]]], $container);
$loader->load([['providers' => []]], $container);

This comment has been minimized.

Copy link
@stof

stof Sep 20, 2019

Collaborator

this legacy test should be removed entirely instead, as it is the test about disabling the ContainerAwareProvider

garak added 2 commits Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.