Skip to content

[SecurityBundle] resolve class name parameter inside AddSecurityVotersPass #23862

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[SecurityBundle] resolve class name parameter inside AddSecurityVotersPass #23862

wants to merge 6 commits into from

Conversation

pjarmalavicius
Copy link
Contributor

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #23733
License MIT

@chalasr chalasr added this to the 3.4 milestone Aug 11, 2017
@chalasr
Copy link
Member

chalasr commented Aug 18, 2017

Continuing discussion from the fixed ticket:

Yes, it's the same as @chalasr suggests, but if we follow symfony best practices, then I would say that we also should write core to follow it. I mean class name parameters are not recommended and maybe one day it will be deprecated. Having check validity compiler pass doesn't depend on class parameter parsing and it also doesn't change what worked before

Resolving parameter in the existing pass could not break anything, only fix something that currently breaks (since %parameter% aren't valid class names).
Also I'm not fond of having a TYPE_OPTIMIZE pass which doesn't optimize actually.
If we want to fix this, I would say we should be consistent with the existing pass(es) doing it already, resolving parameters in the existing pass.

@chalasr
Copy link
Member

chalasr commented Aug 18, 2017

Registering service class names was a convention which has been abandoned, we just stopped recommending to do so, we won't forbid it.

@pjarmalavicius pjarmalavicius changed the title [SecurityBundle] added compiler pass for checking voters validity [SecurityBundle] resolve class name parameter inside AddSecurityVotersPass Aug 21, 2017
stof
stof reviewed Aug 21, 2017
View reviewed changes
src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSecurityVotersPass.php Outdated
if (!is_a($class, VoterInterface::class, true)) {
@trigger_error(sprintf('Using a security.voter tag on a class without implementing the %1$s is deprecated as of 3.4 and will be removed in 4.0. Implement the %1$s instead.', VoterInterface::class), E_USER_DEPRECATED);
}

Copy link
Member

Choose a reason for hiding this comment

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

please don't remove the empty lines we have for readability.

@chalasr
Copy link
Member

chalasr commented Aug 21, 2017

Thank you @pjarmalavicius.

chalasr pushed a commit that referenced this pull request Aug 21, 2017
feature #23862 [SecurityBundle] resolve class name parameter inside A…
a675d80 
…ddSecurityVotersPass (pjarmalavicius)

This PR was squashed before being merged into the 3.4 branch (closes #23862).

Discussion
----------

[SecurityBundle] resolve class name parameter inside AddSecurityVotersPass

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23733
| License       | MIT

Commits
-------

a86bf52 [SecurityBundle] resolve class name parameter inside AddSecurityVotersPass
@chalasr chalasr closed this Aug 21, 2017
This was referenced Oct 18, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug SecurityBundle Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
4 participants
@pjarmalavicius @chalasr @stof @carsonbot