Rename DebugAccessDecisionManager to TraceableAccessDecisionManager #21088

Jean85 · 2016-12-29T09:50:54Z

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

[EDIT] No longer WIP, test passing. Also, test added to preserve BC with the SecurityBundle.

…ymfony#21085)

wouterj · 2016-12-29T09:59:59Z

Tests seem to pass on Travis. So maybe a local problem? I think you're correct about not having to care about BC here as it's @internal.

status: reviewed

(btw, you can ignore fabbot.io's coding style error)

xabbuh

We need to keep the old class name (possibly through a class alias). Otherwise, older versions of the SecurityBundle would break when being used with 3.3/3.4 versions of the Security Core component. We should probably add a test case (the fact that the build suite passed indicates that we are not testing anything/the right thing here).

Jean85 · 2016-12-29T10:22:08Z

Well, it was not a local error but a fail of the Travis build on my fork, here: https://travis-ci.org/Jean85/symfony/builds/187412622

If we need a BC class, I will add it. In fact, mixing versions of the bundles may break stuff like in my build, you're right!

xabbuh · 2016-12-29T10:56:02Z

src/Symfony/Component/Security/Core/Authorization/DebugAccessDecisionManager.php

 */
-class DebugAccessDecisionManager implements AccessDecisionManagerInterface
+class DebugAccessDecisionManager extends TraceableAccessDecisionManager


What about using the same approach that @nicolas-grekas used in #20967 instead?

Nice! Doing it RN!

Test failing locally... Maybe I'm missing something, but how is it supposed to work like that? It's defining the alias before declaring the class, it says that the Traceable class is missing!

Can you push your latest commit so we can take a look at it?

…deprecation

xabbuh · 2016-12-29T15:16:09Z

src/Symfony/Component/Security/Core/Authorization/TraceableAccessDecisionManager.php

+
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+
+class_alias(TraceableAccessDecisionManager::class, DebugAccessDecisionManager::class);


moving this line after the class definition should make tests green

I've chosen a different approach: I've removed the class_exist (since it seems useless to me) and I've moved it into the deprecated file, which seems the right place to me.

Jean85 · 2016-12-30T09:02:02Z

Latests commits are coming from the discussion with @nicolas-grekas in #21096.

xabbuh · 2016-12-30T09:34:26Z

@Jean85 In your test, the DebugAccessDecisionManager class was already loaded in line 47.

Jean85 · 2016-12-30T11:03:50Z

You're right; I've tested locally and it works even if I launch the single tests method, so it's just a test issue, to avoid future regressions.

Maybe it's better if I move the tests to the top of the class, and I use a simple class_exist, to avoid this issue?

xabbuh · 2016-12-30T14:24:49Z

@Jean85 Not sure I completely understood what you meant with your last question. I suggest to simply follow the approach from #20967 though.

Jean85 · 2016-12-30T15:37:41Z

I've reverted to the suggested approach, an rewrote the test to avoid triggering the autoload of the deprecated class.

Sorry if I was so stubborn @xabbuh , now I've rewritten the test to show how my approach was flawed. PHPUnit's assertInstanceOf was doing a class_exist behind the scene, triggering the autoload and hiding my mistake.

xabbuh · 2016-12-30T16:37:00Z

src/Symfony/Component/Security/Core/Tests/Authorization/TraceableAccessDecisionManagerTest.php

@@ -40,4 +41,16 @@ public function provideObjectsAndLogs()
        yield array(array(array('attributes' => array('ATTRIBUTE_1'), 'object' => $x = array(), 'result' => false)), $x);
        yield array(array(array('attributes' => array('ATTRIBUTE_1'), 'object' => $object, 'result' => false)), $object);
    }
+
+    /**
+     * @group this


should be removed

xabbuh · 2016-12-30T16:37:52Z

@Jean85 Don't worry about that. Renaming is actually not so easy and it's always good to think about the approaches we use.

Jean85 · 2016-12-30T16:58:20Z

Thanks! I've removed the annotation that I left behind by mistake!

nicolas-grekas

👍 with minor changes

nicolas-grekas · 2017-01-02T16:37:08Z

src/Symfony/Component/Security/Core/Authorization/DebugAccessDecisionManager.php

- * about the security voters and the decisions made by them.
- *
- * @author Javier Eguiluz <javier.eguiluz@gmail.com>
+ * @author Alessandro Lai <alessandro.lai85@gmail.com>


you should remove this line, there is no authorship to claim here: git will record your contribution

nicolas-grekas · 2017-01-02T16:37:18Z

src/Symfony/Component/Security/Core/Authorization/DebugAccessDecisionManager.php

 *
 * @internal
+ * @deprecated The DebugAccessDecisionManager class is deprecated since version 3.3 and will be removed in 4.0. Use the Symfony\Component\Security\Core\Authorization\TraceableAccessDecisionManager class instead.


the namespace could be removed

nicolas-grekas · 2017-01-02T16:38:31Z

src/Symfony/Component/Security/Core/Tests/Authorization/TraceableAccessDecisionManagerTest.php

+    {
+        $adm = new TraceableAccessDecisionManager(new AccessDecisionManager());
+
+        if (! $adm instanceof DebugAccessDecisionManager) {


extra space to be removed after !

nicolas-grekas · 2017-01-02T16:40:04Z

src/Symfony/Component/Security/Core/Tests/Authorization/TraceableAccessDecisionManagerTest.php

+        $adm = new TraceableAccessDecisionManager(new AccessDecisionManager());
+
+        if (! $adm instanceof DebugAccessDecisionManager) {
+            $this->fail('Doesn\'t work, BC with SecurityBundle broken, see PR #21088');


we don't reference github, git blame is used instead to look for historical things. I propose:
$this->fail('For BC, TraceableAccessDecisionManager must be an instance of DebugAccessDecisionManager');

Jean85 · 2017-01-02T16:54:49Z

Fixes committed! The style checker is having some false positive I think...

xabbuh · 2017-01-02T17:58:44Z

src/Symfony/Component/Security/Core/Authorization/DebugAccessDecisionManager.php

- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */


This should IMO not be removed.

Jean85 · 2017-01-03T08:20:27Z

@xabbuh done!

xabbuh · 2017-01-03T09:36:22Z

👍

Status: Reviewed

nicolas-grekas · 2017-01-03T09:52:41Z

Thank you @Jean85.

…sionManager (Jean85) This PR was squashed before being merged into the 3.3-dev branch (closes #21088). Discussion ---------- Rename DebugAccessDecisionManager to TraceableAccessDecisionManager | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21085 | License | MIT [EDIT] No longer WIP, test passing. Also, test added to preserve BC with the SecurityBundle. Commits ------- c5e0e59 Rename DebugAccessDecisionManager to TraceableAccessDecisionManager

Rename DebugAccessDecisionManager to TraceableAccessDecisionManager (s…

89f64af

…ymfony#21085)

carsonbot added the Status: Needs Review label Dec 29, 2016

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 29, 2016

xabbuh requested changes Dec 29, 2016

View reviewed changes

Add deprecated DebugAccessDecisionManager class alias

a579ce8

xabbuh reviewed Dec 29, 2016

View reviewed changes

Attempt at following example from symfony#20967 for class rename and …

046acdd

…deprecation

xabbuh reviewed Dec 29, 2016

View reviewed changes

Fix class_alias moving it to the deprecated class file

1e29773

Jean85 changed the title ~~[WIP] Rename DebugAccessDecisionManager to TraceableAccessDecisionManager~~ Rename DebugAccessDecisionManager to TraceableAccessDecisionManager Dec 29, 2016

Add assertion to test instanceof of deprecated class

41f995e

Jean85 mentioned this pull request Dec 29, 2016

[DependencyInjection] register alias after defining the class #21096

Merged

Add test for reverse BC preservation

78c8e08

Fix test for BC, revert to suggested approach

83207c4

xabbuh reviewed Dec 30, 2016

View reviewed changes

xabbuh approved these changes Dec 30, 2016

View reviewed changes

Remove unused group annotation on test

9843ddb

nicolas-grekas reviewed Jan 2, 2017

View reviewed changes

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

Minor fixes to comply to review

7c39010

xabbuh reviewed Jan 2, 2017

View reviewed changes

Restoring copyright notice

3b9c24c

nicolas-grekas closed this Jan 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename DebugAccessDecisionManager to TraceableAccessDecisionManager #21088

Rename DebugAccessDecisionManager to TraceableAccessDecisionManager #21088

Jean85 commented Dec 29, 2016 •

edited

Loading

wouterj commented Dec 29, 2016 •

edited

Loading

xabbuh left a comment

Jean85 commented Dec 29, 2016

xabbuh Dec 29, 2016

Jean85 Dec 29, 2016

Jean85 Dec 29, 2016

xabbuh Dec 29, 2016

Jean85 Dec 29, 2016

xabbuh Dec 29, 2016

Jean85 Dec 29, 2016

Jean85 commented Dec 30, 2016

xabbuh commented Dec 30, 2016

Jean85 commented Dec 30, 2016

xabbuh commented Dec 30, 2016 •

edited

Loading

Jean85 commented Dec 30, 2016

xabbuh Dec 30, 2016

xabbuh commented Dec 30, 2016

Jean85 commented Dec 30, 2016

nicolas-grekas left a comment

nicolas-grekas Jan 2, 2017

nicolas-grekas Jan 2, 2017

nicolas-grekas Jan 2, 2017

nicolas-grekas Jan 2, 2017

Jean85 commented Jan 2, 2017

xabbuh Jan 2, 2017

Jean85 commented Jan 3, 2017

xabbuh commented Jan 3, 2017

nicolas-grekas commented Jan 3, 2017


		use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;

		class_alias(TraceableAccessDecisionManager::class, DebugAccessDecisionManager::class);

Rename DebugAccessDecisionManager to TraceableAccessDecisionManager #21088

Rename DebugAccessDecisionManager to TraceableAccessDecisionManager #21088

Conversation

Jean85 commented Dec 29, 2016 • edited Loading

wouterj commented Dec 29, 2016 • edited Loading

xabbuh left a comment

Choose a reason for hiding this comment

Jean85 commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jean85 commented Dec 30, 2016

xabbuh commented Dec 30, 2016

Jean85 commented Dec 30, 2016

xabbuh commented Dec 30, 2016 • edited Loading

Jean85 commented Dec 30, 2016

Choose a reason for hiding this comment

xabbuh commented Dec 30, 2016

Jean85 commented Dec 30, 2016

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jean85 commented Jan 2, 2017

Choose a reason for hiding this comment

Jean85 commented Jan 3, 2017

xabbuh commented Jan 3, 2017

nicolas-grekas commented Jan 3, 2017

Jean85 commented Dec 29, 2016 •

edited

Loading

wouterj commented Dec 29, 2016 •

edited

Loading

xabbuh commented Dec 30, 2016 •

edited

Loading