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

EZP-27249: Fix routerpass to use defined class #88

Merged
merged 3 commits into from
Apr 13, 2017
Merged

EZP-27249: Fix routerpass to use defined class #88

merged 3 commits into from
Apr 13, 2017

Conversation

wizhippo
Copy link
Contributor

Follow kernel and no longer use router.class parameter

@emodric
Copy link
Collaborator

emodric commented Apr 13, 2017

@wizhippo You tested and it works?

If so, +1, apart from my comment.

@@ -22,6 +23,10 @@ public function process(ContainerBuilder $container)
return;
}

$container
->findDefinition('router.default')
->setClass(DefaultRouter::class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use a FQCN as string, since legacy bridge still supports PHP 5.4.

@emodric
Copy link
Collaborator

emodric commented Apr 13, 2017

@wizhippo Now I see there's already $defaultRouterDef variable just below your code. You can then add the setClass call to that variable instead of using findDefinition :)

@wizhippo
Copy link
Contributor Author

@emodric Working here so far.

@@ -6,7 +6,6 @@ parameters:
ezpublish.default_router.legacy_aware_routes: ['_ezpublishLegacyTreeMenu', 'ezpublish_rest_', '_ezpublishPreviewContent', '_wdt', '_profiler', '_assetic']

# Core overrides
router.class: eZ\Bundle\EzPublishLegacyBundle\Routing\DefaultRouter
ezpublish.security.login_listener.class: eZ\Bundle\EzPublishLegacyBundle\Security\SecurityListener
security.authentication.listener.rememberme.class: eZ\Bundle\EzPublishLegacyBundle\Security\RememberMeListener
Copy link
Contributor

Choose a reason for hiding this comment

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

if this comes from symfony, then this is broken on 3.x as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep. We can fix it in separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting another PR just for the parameter removal?

Not quite sure I follow. As the compiler pass now sets the class, this parameter is not used anyway.

Let me know what you want done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wizhippo I think @andrerom is referring to security.authentication.listener.rememberme.class parameter which comes from Symfony, so Legacy Bridge will not work on Symfony 3.x because this parameter was removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it can be done in separate PR, just noticed it while reviewing here ;)

@emodric emodric merged commit e66c3df into ezsystems:master Apr 13, 2017
@emodric
Copy link
Collaborator

emodric commented Apr 13, 2017

Thanks @wizhippo 👍

@wizhippo wizhippo deleted the paramremoval branch April 13, 2017 17:01
@emodric emodric mentioned this pull request Apr 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants