[SecurityBundle] Lazy load request matchers in FirewallMap #21451

chalasr · Jan 29, 2017

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/a

nicolas-grekas · Jan 29, 2017

👍

fabpot · Jan 30, 2017

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php

@@ -28,7 +28,7 @@ class FirewallMap implements FirewallMapInterface
    protected $map;
    private $contexts;

-    public function __construct(ContainerInterface $container, array $map)
+    public function __construct(ContainerInterface $container, $map)


map is protected, not private. So, that's a BC break.

Is there any possible upgrade path?

Or is it acceptable if documented? I'll close if nothing can be done

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.

perhaps JMS security bundle

Even not. Let's close then

fabpot · Jan 30, 2017

Instead of closing it, what about documenting the change in the CHANGELOG/UPGRADE files and target 4.0?

chalasr · Jan 30, 2017

Let's do that. Target and CHANGELOG/UPGRADE files updated, this should be removed from the 3.3 milestone

fabpot · Jan 30, 2017

Changed the milestone to 4.0.

nicolas-grekas · Jan 30, 2017

But we can create a smooth upgrade path: let's deprecate $map.
See #20769 for inspiration.

chalasr · Jan 30, 2017

@nicolas-grekas Updated, thank you.

@fabpot Is it ok for you?

fabpot · Jan 30, 2017

👍

nicolas-grekas · Jan 30, 2017

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php

-    protected $container;
+    /**
+     * @deprecated since version 3.3, to be removed in 4.0 alongside with magic methods below
+     */
    protected $map;


should be made private

Better indeed :)

nicolas-grekas · Jan 30, 2017

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php

+    public function __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);


should we do an $this->map = iterator_to_array($this->contextMap, false) when appropriate?

also: contextMap is private, so it can't be used "instead". any other suggestions?

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

nicolas-grekas · Jan 30, 2017

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php

+    public function __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);


~~use ""~~

chalasr · Jan 30, 2017

Milestone should be changed, again :)

nicolas-grekas · Jan 30, 2017

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php

+    /**
+     * @deprecated since version 3.3, to be removed in 4.0 alongside with magic methods below
+     */
+    private $map;
    protected $container;


so, no deprecation of this prop?

Another IteratorArgument as locator could do the job. WDYT?

Done in my last commit, let me know if I should revert or keep

Oops, nevermind. That does not work. Definitely needs #20658 for that. Sorry for the noise

nicolas-grekas · Jan 31, 2017

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php

+    /**
+     * @deprecated since version 3.3, to be removed in 4.0 alongside with magic methods below
+     */
+    private $map;
    protected $container;


Still, does this prop need to be protected?

nicolas-grekas · Jan 31, 2017

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php

+        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->contextMap instanceof \Iterator) {


missing null === $this->map check?

chalasr · Jan 31, 2017

@nicolas-grekas deprecated using $container and addressed your comments.

nicolas-grekas · Jan 31, 2017

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php

-        $this->map = $map;
-        $this->contexts = new \SplObjectStorage();
+        $this->contextCache = new \SplObjectStorage();
+        $this->contextMap = $contextMap;


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

Reverted $contextCache to $contexts. About $contextMap if we want to keep it called $map we can't do null === $this->map and return the iterator as array, right? I think we can't

nicolas-grekas

👍

fabpot · Jan 31, 2017

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php

@@ -22,13 +22,94 @@
 *
 * @author Johannes M. Schmitt <schmittjoh@gmail.com>
 */
-class FirewallMap implements FirewallMapInterface
+class FirewallMap extends _FireWallMap implements FirewallMapInterface


Typo: should be _FirewallMap

fabpot · Jan 31, 2017

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

+3.3.0
+-----
+
+ * Deprecated the `FirewallMap::$map`  and `$container` properties.


extra space before and

fabpot · Jan 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/a Commits ------- 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/a Commits ------- 16a2fcf Remove deprecated container injections and compiler passes

carsonbot added Status: Needs Review SecurityBundle Feature labels Jan 29, 2017

chalasr force-pushed the firewall/lazy-contexts branch from 243e145 to 8a34e7d Compare January 29, 2017 12:27

nicolas-grekas added this to the 3.3 milestone Jan 29, 2017

fabpot reviewed Jan 30, 2017

View reviewed changes

chalasr closed this Jan 30, 2017

chalasr deleted the firewall/lazy-contexts branch January 30, 2017 16:21

chalasr restored the firewall/lazy-contexts branch January 30, 2017 16:23

chalasr reopened this Jan 30, 2017

chalasr force-pushed the firewall/lazy-contexts branch 2 times, most recently from f9db833 to 81e0d5b Compare January 30, 2017 16:32

fabpot modified the milestones: 4.0, 3.3 Jan 30, 2017

chalasr force-pushed the firewall/lazy-contexts branch 2 times, most recently from 46cb2c3 to 8b24706 Compare January 30, 2017 17:11

chalasr force-pushed the firewall/lazy-contexts branch 2 times, most recently from 4212d7e to 62dfc3d Compare January 30, 2017 17:16

nicolas-grekas reviewed Jan 30, 2017

View reviewed changes

chalasr force-pushed the firewall/lazy-contexts branch from 62dfc3d to 2dc17a8 Compare January 30, 2017 18:19

chalasr force-pushed the firewall/lazy-contexts branch from 2dc17a8 to f6c13a2 Compare January 30, 2017 19:34

nicolas-grekas added this to the 3.3 milestone Jan 30, 2017

nicolas-grekas removed this from the 4.0 milestone Jan 30, 2017

nicolas-grekas reviewed Jan 30, 2017

View reviewed changes

chalasr force-pushed the firewall/lazy-contexts branch 2 times, most recently from 311f90f to b51d617 Compare January 30, 2017 21:06

nicolas-grekas reviewed Jan 31, 2017

View reviewed changes

chalasr force-pushed the firewall/lazy-contexts branch 3 times, most recently from 6625b75 to a171dbc Compare January 31, 2017 08:24

nicolas-grekas reviewed Jan 31, 2017

View reviewed changes

chalasr force-pushed the firewall/lazy-contexts branch 2 times, most recently from 7c27a67 to d2e0798 Compare January 31, 2017 19:42

nicolas-grekas approved these changes Jan 31, 2017

View reviewed changes

fabpot reviewed Jan 31, 2017

View reviewed changes

[Security] Lazy load request matchers

5b72cf6

chalasr force-pushed the firewall/lazy-contexts branch from d2e0798 to 5b72cf6 Compare January 31, 2017 20:20

fabpot merged commit 5b72cf6 into symfony:master Jan 31, 2017

javiereguiluz mentioned this pull request Feb 12, 2017

Request matchers in FirewallMap are lazy loaded symfony/symfony-docs#7474

Closed

chalasr mentioned this pull request Feb 23, 2017

[DependencyInjection] Use a service locator in AddConstraintValidatorsPass #21730

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

chalasr deleted the firewall/lazy-contexts branch May 10, 2017 16:45

chalasr mentioned this pull request May 18, 2017

Remove deprecated container injections and compiler passes #22749

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[SecurityBundle] Lazy load request matchers in FirewallMap #21451

[SecurityBundle] Lazy load request matchers in FirewallMap #21451

Uh oh!

Conversation

chalasr commented Jan 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jan 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jan 30, 2017

Uh oh!

chalasr commented Jan 30, 2017

Uh oh!

fabpot commented Jan 30, 2017

Uh oh!

nicolas-grekas commented Jan 30, 2017

Uh oh!

chalasr commented Jan 30, 2017

Uh oh!

fabpot commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commented Jan 29, 2017 •

edited

Loading

chalasr Jan 30, 2017 •

edited

Loading

chalasr Jan 31, 2017 •

edited

Loading