Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[SecurityBundle] Lazy load request matchers in FirewallMap#21451
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
243e145 to8a34e7dComparenicolas-grekas commentedJan 29, 2017
👍 |
| private$contexts; | ||
| publicfunction__construct(ContainerInterface$container,array$map) | ||
| publicfunction__construct(ContainerInterface$container,$map) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
map is protected, not private. So, that's a BC break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there any possible upgrade path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or is it acceptable if documented? I'll close if nothing can be done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I doubt many users are actually overriding this class (perhaps JMS security bundle?) but I would do it only in 4.0 with proper docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
perhaps JMS security bundle
Even not. Let's close then
fabpot commentedJan 30, 2017
Instead of closing it, what about documenting the change in the CHANGELOG/UPGRADE files and target 4.0? |
f9db833 to81e0d5bComparechalasr commentedJan 30, 2017
Let's do that. Target and CHANGELOG/UPGRADE files updated, this should be removed from the 3.3 milestone |
fabpot commentedJan 30, 2017
Changed the milestone to 4.0. |
nicolas-grekas commentedJan 30, 2017
But wecan create a smooth upgrade path: let's deprecate |
46cb2c3 to8b24706Comparechalasr commentedJan 30, 2017
@nicolas-grekas Updated, thank you. @fabpot Is it ok for you? |
4212d7e to62dfc3dComparefabpot commentedJan 30, 2017
👍 |
| /** | ||
| * @deprecated since version 3.3, to be removed in 4.0 alongside with magic methods below | ||
| */ | ||
| protected$map; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be made private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Better indeed :)
| publicfunction__get($name) | ||
| { | ||
| if ('map' ===$name) { | ||
| @trigger_error(sprintf('Using the "%1$s::$map" property is deprecated since version 3.3 as it will be removed in 4.0. Use "%1$s::$contextMap" instead.',__CLASS__),E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should we do an$this->map = iterator_to_array($this->contextMap, false) when appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
also: contextMap is private, so it can't be used "instead". any other suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should we do an $this->map = iterator_to_array($this->contextMap, false) when appropriate?
👍 though you say$use_keys = false but keys are important here, done
also: contextMap is private, so it can't be used "instead". any other suggestions?
nope, fixed
| publicfunction__isset($name) | ||
| { | ||
| if ('map' ===$name) { | ||
| @trigger_error(sprintf('Using the "%1$s::$map" property is deprecated since version 3.3 as it will be removed in 4.0. Use ""',__CLASS__),E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
use ""
62dfc3d to2dc17a8Comparechalasr commentedJan 30, 2017
Milestone should be changed, again :) |
2dc17a8 tof6c13a2Compare| * @deprecated since version 3.3, to be removed in 4.0 alongside with magic methods below | ||
| */ | ||
| private$map; | ||
| protected$container; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
so, no deprecation of this prop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Another IteratorArgument as locator could do the job. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done in my last commit, let me know if I should revert or keep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops, nevermind. That does not work. Definitely needs#20658 for that. Sorry for the noise
311f90f tob51d617Compare| * @deprecated since version 3.3, to be removed in 4.0 alongside with magic methods below | ||
| */ | ||
| private$map; | ||
| protected$container; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Still, does this prop need to be protected?
| if ('map' ===$name) { | ||
| @trigger_error(sprintf('Using the "%s::$map" property is deprecated since version 3.3 as it will be removed in 4.0.',__CLASS__),E_USER_DEPRECATED); | ||
| if ($this->contextMapinstanceof \Iterator) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missingnull === $this->map check?
6625b75 toa171dbcComparechalasr commentedJan 31, 2017
@nicolas-grekas deprecated using |
| $this->map =$map; | ||
| $this->contexts =new \SplObjectStorage(); | ||
| $this->contextCache =new \SplObjectStorage(); | ||
| $this->contextMap =$contextMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do we really need to change the name of these props? we'd better keep it as is for a small diff if we can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Reverted$contextCache to$contexts. About$contextMap if we want to keep it called$map we can't donull === $this->map and return the iterator as array, right? I think we can't
7c27a67 tod2e0798Compare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
| * @author Johannes M. Schmitt <schmittjoh@gmail.com> | ||
| */ | ||
| class FirewallMapimplements FirewallMapInterface | ||
| class FirewallMapextends _FireWallMapimplements FirewallMapInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Typo: should be_FirewallMap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
good catch
| 3.3.0 | ||
| ----- | ||
| * Deprecated the`FirewallMap::$map` and`$container` properties. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
extra space beforeand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
d2e0798 to5b72cf6Comparefabpot commentedJan 31, 2017
Thank you@chalasr. |
…lMap (chalasr)This PR was merged into the 3.3-dev branch.Discussion----------[SecurityBundle] Lazy load request matchers in FirewallMap| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aCommits-------5b72cf6 [Security] Lazy load request matchers
…sses (chalasr)This PR was merged into the 4.0-dev branch.Discussion----------Remove deprecated container injections and compiler passes| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets | ~~#21451#21625#21284#22010~~#22805| License | MIT| Doc PR | n/aCommits-------16a2fcf Remove deprecated container injections and compiler passes
Uh oh!
There was an error while loading.Please reload this page.