[DependencyInjection][Filesystem][Security][SecurityBundle] remove deprecated features #24364

xabbuh · 2017-09-28T16:43:27Z

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

nicolas-grekas · 2017-09-28T16:48:04Z

src/Symfony/Component/Security/Http/Firewall/ContextListener.php

@@ -76,7 +76,7 @@ public function __construct(TokenStorageInterface $tokenStorage, iterable $userP
     */
    public function setLogoutOnUserChange($logoutOnUserChange)
    {
-        $this->logoutOnUserChange = (bool) $logoutOnUserChange;
+        // no-op, to be deprecated in 4.1


should it throw if false is provided?

Not sure, in the deprecation message of the related option we tell the user that it will be ignored in 4.0. So, I think we can do the same with the method.

On v3.0, in ChoiceType, we threw an exception when "$choices_as_values !== true" in a similar case. Is there any difference here?

xabbuh · 2017-09-28T19:53:15Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

@@ -270,8 +258,6 @@ private function getAutowiredReference(TypedReference $reference, $deprecationMe
        if (isset($this->autowired[$type])) {
            return $this->autowired[$type] ? new TypedReference($this->autowired[$type], $type) : null;
        }
-
-        return $this->createAutowiredDefinition($type);


@dunglas @nicolas-grekas @GuilhemN Can you confirm that this is the proper fix here?

Tobion · 2017-09-29T00:35:16Z

src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php

-    {
-        @trigger_error('Calling InlineServiceDefinitionsPass::getInlinedServiceIds() is deprecated since Symfony 3.4 and will be removed in 4.0.', E_USER_DEPRECATED);
-
-        return $this->inlinedServiceIds;


shouldn't the property be removed as well? otherwise I don't see the point of it

linaori · 2017-09-29T11:48:54Z

src/Symfony/Component/Security/Http/Firewall/ContextListener.php

-        $this->logoutOnUserChange = (bool) $logoutOnUserChange;
+        // to be deprecated in 4.1
+        if (true !== $logoutOnUserChange) {
+            throw new \RuntimeException(sprintf('The %s() should not be used with a value other than true.', __METHOD__));


Same should be done in the bundle configuration

In fact, I am not sure anymore if we should really throw the exception here. I mean this is now a code path that changes its behaviour compared to Symfony 3.4 without triggering a deprecation before.

In theory this would shield it when people set it to false and didn't adhere to the deprecation from 3.4. In theory nothing breaks though, except that a useless method call is executed.

Could instead make it trigger just a deprecation in 4.1

chalasr · 2017-09-30T18:13:04Z

src/Symfony/Component/Security/Http/Firewall/ContextListener.php

-        $this->logoutOnUserChange = (bool) $logoutOnUserChange;
+        // to be deprecated in 4.1
+        if (true !== $logoutOnUserChange) {
+            throw new \RuntimeException(sprintf('The %s() should not be used with a value other than true.', __METHOD__));


missing method after %s()

nicolas-grekas · 2017-10-02T16:22:02Z

rebase needed (the DI part has been removed in another PR)

xabbuh · 2017-10-03T21:13:40Z

rebased and changelog files updated too, failures are not related

nicolas-grekas · 2017-10-03T21:20:36Z

Are you sure failures are not related? It looks like this may break 3.4, isn't it?

xabbuh · 2017-10-03T22:53:00Z

I see two solutions: Either we don't throw the exception in ContextListener::setLogoutOnUserChange() and silently ignore any value being passed (probably deprecating any value that is not true in 4.1) or we need to update the SecurityExtension in 3.4 to not add the method call when the bundle is used with the Security component in version 4.0.

xabbuh · 2017-10-05T11:56:02Z

@symfony/deciders Any preferences how we resolve #24364 (comment)? Basically, this is caused by the change I made based on the discussion in #24364 (comment).

The difference compared to the example in the Form component mentioned by @nicolas-grekas is that we did not have any cross-component usage of the choices_as_values option with a value other than true in Symfony 2.8.

chalasr · 2017-10-05T12:23:16Z

My preference goes for the no-op in 4.0, deprecating the method in 4.1.

chalasr · 2017-10-05T12:43:14Z

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

@@ -14,6 +14,8 @@ CHANGELOG
 * removed command `init:acl` along with `InitAclCommand` class
 * removed `acl` configuration key and related services, use symfony/acl-bundle instead
 * removed auto picking the first registered provider when no configured provider on a firewall and ambiguous
+ * The firewall option `logout_on_user_change` is now always true, which will trigger a logout if the user changes
+   between requests.


We need a note for the switch_user.stateless option. I was about to use:

* made the `switch_user.stateless` option `true` when firewall is stateless

chalasr · 2017-10-05T12:49:35Z

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php

-     * @group legacy
-     * @expectedDeprecation Firewall "some_firewall" is configured as "stateless" but the "switch_user.stateless" key is set to false. Both should have the same value, the firewall's "stateless" value will be used as default value for the "switch_user.stateless" key in 4.0.
-     */
-    public function testSwitchUserNotStatelessOnStatelessFirewall()


I suggest to rename this test, remove the legacy flag and add the following assertion

$this->assertTrue($container->getDefinition('security.authentication.switchuser_listener.default')->getArgument(9));

xabbuh · 2017-10-06T10:15:08Z

@chalasr done

chalasr · 2017-10-06T12:16:54Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php

@@ -202,7 +202,7 @@ private function addFirewallsSection(ArrayNodeDefinition $rootNode, array $facto
            ->booleanNode('stateless')->defaultFalse()->end()
            ->scalarNode('context')->cannotBeEmpty()->end()
            ->booleanNode('logout_on_user_change')
-                ->defaultFalse()
+                ->defaultTrue()
                ->info('When true, it will trigger a logout for the user if something has changed. This will be the default behavior as of Syfmony 4.0.')


Last sentence should be removed

fabpot · 2017-10-06T14:39:12Z

Thank you @xabbuh.

…ndle] remove deprecated features (xabbuh) This PR was merged into the 4.0-dev branch. Discussion ---------- [DependencyInjection][Filesystem][Security][SecurityBundle] remove deprecated features | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Commits ------- 45dd40c remove deprecated features

…deprecated (chalasr) This PR was merged into the 4.1 branch. Discussion ---------- [Security] The logout_on_user_change firewall option is deprecated Since symfony/symfony#24364 Commits ------- 18c8f91 [Security] The logout_on_user_change firewall option is deprecated

xabbuh added this to the 4.0 milestone Sep 28, 2017

carsonbot added Status: Needs Review DependencyInjection Filesystem Security SecurityBundle BC Break labels Sep 28, 2017

nicolas-grekas reviewed Sep 28, 2017

View reviewed changes

xabbuh force-pushed the remove-deprecated-features branch 2 times, most recently from 64c1890 to aa33204 Compare September 28, 2017 19:29

xabbuh mentioned this pull request Sep 28, 2017

[HttpKernel] Remove bundle inheritance #24161

Merged

xabbuh force-pushed the remove-deprecated-features branch from aa33204 to 6802cbf Compare September 28, 2017 19:52

xabbuh commented Sep 28, 2017

View reviewed changes

Tobion reviewed Sep 29, 2017

View reviewed changes

xabbuh force-pushed the remove-deprecated-features branch from 6802cbf to cf00efa Compare September 29, 2017 08:34

linaori reviewed Sep 29, 2017

View reviewed changes

chalasr reviewed Sep 30, 2017

View reviewed changes

xabbuh force-pushed the remove-deprecated-features branch 5 times, most recently from a4cc3d6 to 85a7ef4 Compare October 3, 2017 20:32

nicolas-grekas approved these changes Oct 3, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 3, 2017

chalasr reviewed Oct 5, 2017

View reviewed changes

xabbuh force-pushed the remove-deprecated-features branch 2 times, most recently from 5cdd00c to 75f9c3f Compare October 6, 2017 09:50

chalasr approved these changes Oct 6, 2017

View reviewed changes

remove deprecated features

45dd40c

xabbuh force-pushed the remove-deprecated-features branch from 75f9c3f to 45dd40c Compare October 6, 2017 12:47

fabpot approved these changes Oct 6, 2017

View reviewed changes

fabpot merged commit 45dd40c into symfony:master Oct 6, 2017

xabbuh deleted the remove-deprecated-features branch October 6, 2017 14:46

fabpot mentioned this pull request Oct 19, 2017

Release v4.0.0-BETA1 #24612

Merged

chalasr mentioned this pull request Oct 28, 2018

[Security] The logout_on_user_change firewall option is deprecated symfony/symfony-docs#10596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection][Filesystem][Security][SecurityBundle] remove deprecated features #24364

[DependencyInjection][Filesystem][Security][SecurityBundle] remove deprecated features #24364

xabbuh commented Sep 28, 2017

nicolas-grekas Sep 28, 2017

xabbuh Sep 28, 2017

nicolas-grekas Sep 28, 2017

xabbuh Sep 28, 2017

nicolas-grekas Sep 28, 2017

Tobion Sep 29, 2017

linaori Sep 29, 2017

xabbuh Oct 3, 2017

linaori Oct 3, 2017

chalasr Sep 30, 2017

nicolas-grekas commented Oct 2, 2017

xabbuh commented Oct 3, 2017

nicolas-grekas commented Oct 3, 2017

xabbuh commented Oct 3, 2017

xabbuh commented Oct 5, 2017

chalasr commented Oct 5, 2017

chalasr Oct 5, 2017

chalasr Oct 5, 2017

xabbuh commented Oct 6, 2017

chalasr Oct 6, 2017

xabbuh Oct 6, 2017

fabpot commented Oct 6, 2017

[DependencyInjection][Filesystem][Security][SecurityBundle] remove deprecated features #24364

[DependencyInjection][Filesystem][Security][SecurityBundle] remove deprecated features #24364

Conversation

xabbuh commented Sep 28, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Oct 2, 2017

xabbuh commented Oct 3, 2017

nicolas-grekas commented Oct 3, 2017

xabbuh commented Oct 3, 2017

xabbuh commented Oct 5, 2017

chalasr commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 6, 2017