NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[HttpFoundation] Deprecate compatibility with PHP <5.4 sessions#24239

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

Jump to bottom

Closed

afurculita wants to merge3 commits intosymfony:3.4fromAlexanderForks:3.4

Closed

[HttpFoundation] Deprecate compatibility with PHP <5.4 sessions#24239

afurculita wants to merge3 commits intosymfony:3.4fromAlexanderForks:3.4

Conversation

Copy link

Contributor

afurculita commentedSep 17, 2017•
edited
Loading

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

This PR removes functionality added in Symfony 2.1 as a compatibility layer with sessions from PHP <5.4.

Fix tests

carsonbot added Status: Needs Review HttpFoundation Deprecation labels

Sep 17, 2017

afurculita force-pushed the3.4 branch fromf3a5b8e to1078645Compare

September 17, 2017 13:35

nicolas-grekas added this to the3.4 milestone

Sep 17, 2017

nicolas-grekas reviewed

Sep 17, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php

		publicfunctionsetSaveHandler($saveHandler =null)
		{
		if (!$saveHandlerinstanceof AbstractProxy &&
		!$saveHandlerinstanceof NativeSessionHandler &&

Copy link

Member

nicolas-grekasSep 17, 2017•
edited
Loading

Choose a reason for hiding this comment

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

should be reverted as that's a BC break, isn't it?

Copy link

ContributorAuthor

afurculitaSep 17, 2017

Choose a reason for hiding this comment

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

Nope, since Symfony 3.0,NativeSessionHandler always extends\SessionHandler. Thus now the rule!$saveHandler instanceof NativeSessionHandler is covered by!$saveHandler instanceof \SessionHandlerInterface

afurculita force-pushed the3.4 branch 2 times, most recently from640661c to71a92a2Compare

September 17, 2017 15:24

Taluu reviewed

Sep 17, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php

		*/

		namespaceSymfony\Component\HttpFoundation\Session\Storage\Proxy;

Copy link

Contributor

TaluuSep 17, 2017

Choose a reason for hiding this comment

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

shouldn't you add a deprecated notice here ?

Copy link

ContributorAuthor

afurculitaSep 17, 2017

Choose a reason for hiding this comment

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

I had one, but removed it becauseAbstractProxy is extended bySessionHandlerProxy and this will make the tests that useSessionHandlerProxy to not pass. I can't mark these tests aslegacy. What's a good solution to have a deprecation notice here and tests pass?

afurculita commented

Sep 17, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/Proxy/SessionHandlerProxy.php

		*
		* @author Drak <drak@zikula.org>
		*/
		class SessionHandlerProxyextends AbstractProxyimplements \SessionHandlerInterface

Copy link

ContributorAuthor

afurculitaSep 17, 2017•
edited
Loading

Choose a reason for hiding this comment

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

I would also move this class from theProxy folder toHandler. What do you think,@nicolas-grekas ?

Copy link

Member

nicolas-grekasSep 19, 2017

Choose a reason for hiding this comment

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

That would need deprecating this class in favor of the new one, and allow triggering a deprecation in AbstractProxy also. LGTM.

Copy link

Contributor

TobionSep 22, 2017

Choose a reason for hiding this comment

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

I think theSessionHandlerProxy can be deprecated as well. When we only support\SessionHandlerInterface anyway, the proxy does not provide any value anymore AFAIK. I was meant as a way to make\SessionHandlerInterface and others behave the same.

Copy link

ContributorAuthor

afurculitaSep 23, 2017

Choose a reason for hiding this comment

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

I was thinking the same. The extra methods thatSessionHandlerProxy is having can easily be moved toNativeSessionStorage as they are relevant only for a native session storage.

sstok reviewed

Sep 18, 2017

View reviewed changes

Copy link

Contributor

sstok left a comment

Choose a reason for hiding this comment

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

Some minor comments on the changelog, but it seems the HttpFoundation also has this rather strange commentary usage.

UPGRADE-3.4.md Outdated

		`TranslationDebugCommand`, `TranslationUpdateCommand`, `XliffLintCommand`
		and `YamlLintCommand` classes have been marked as final

		HttpKernel

Copy link

Contributor

sstokSep 18, 2017

Choose a reason for hiding this comment

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

Missing capital in the sentence (see other lines in this file).

And each line is separated by a single white line.

UPGRADE-3.4.md

		* the `Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy::isWrapper()`
		method has been deprecated and will be removed in 4.0. You can check explicitly if the proxy wraps
		a `\SessionHandler` instance.
		* `NativeSessionStorage::setSaveHandler()` now takes an instance of `\SessionHandlerInterface` as argument.

Copy link

Contributor

sstokSep 18, 2017

Choose a reason for hiding this comment

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

Duplicate header (wrong rebase?)

stof requested changes

Sep 18, 2017

View reviewed changes

UPGRADE-3.4.md


		* `AssetsInstallCommand`, `CacheClearCommand`, `CachePoolClearCommand`,
		`EventDispatcherDebugCommand`, `RouterDebugCommand`, `RouterMatchCommand`,
		`TranslationDebugCommand`, `TranslationUpdateCommand`, `XliffLintCommand`

Copy link

Member

stofSep 18, 2017

Choose a reason for hiding this comment

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

This header should beHttpFoundation

Copy link

ContributorAuthor

afurculitaSep 18, 2017

Choose a reason for hiding this comment

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

Fixed

afurculita force-pushed the3.4 branch 2 times, most recently from845d5a6 tof275707Compare

September 18, 2017 10:31

Copy link

ContributorAuthor

afurculita commentedSep 18, 2017

Comments addressed :)

nicolas-grekas approved these changes

Sep 19, 2017

View reviewed changes

UPGRADE-3.4.md Outdated

		and `YamlLintCommand` classes have been marked as final

		HttpFoundation
		----------

Copy link

Member

nicolas-grekasSep 19, 2017

Choose a reason for hiding this comment

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

wrong number of dashes :)

Copy link

Member

xabbuh commentedSep 20, 2017

Please also update theUPGRADE-4.0.md file.

afurculita force-pushed the3.4 branch 2 times, most recently from415ddf0 to46c0740Compare

September 24, 2017 19:33

Copy link

ContributorAuthor

afurculita commentedSep 24, 2017

Ready for review. I've also deprecatedSessionHandlerProxy

Alexandru Furculitaand others added2 commits

September 24, 2017 22:38

[HttpFoundation] Deprecate <5.4 session functionality

73a1235

Signed-off-by: Alexandru Furculita <alex@furculita.net>

Deprecate SessonHanderProxy

831c006

Signed-off-by: Alexandru Furculita <alex@furculita.net>

afurculita force-pushed the3.4 branch from46c0740 to831c006Compare

September 24, 2017 19:49

Tobion reviewed

Sep 25, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/NativeSessionStorageTest.php Outdated

		$storage =$this->getStorage();
		$storage->setSaveHandler();
		$this->assertInstanceOf('Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy',$storage->getSaveHandler());
		$this->assertInstanceOf(SessionHandlerProxy::class,$storage->getSaveHandler());

Copy link

Contributor

TobionSep 25, 2017

Choose a reason for hiding this comment

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

we don't change that to avoid conflicts unless the line needs to be touched anyway. so this should be reverted.

Tobion reviewed

Sep 25, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/PhpBridgeSessionStorage.php Outdated

		*
		* @param AbstractProxy\|NativeSessionHandler\|\SessionHandlerInterface\|null $handler
		* @param MetadataBag $metaBag MetadataBag
		* @param AbstractProxy\|\SessionHandlerInterface\|null $handler

Copy link

Contributor

TobionSep 25, 2017

Choose a reason for hiding this comment

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

Argument types that are deprecated should be removed from the doc

Tobion reviewed

Sep 25, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php Outdated

		* @param AbstractProxy\|NativeSessionHandler\|\SessionHandlerInterface\|null $handler
		* @param MetadataBag$metaBag MetadataBag
		* @param array $options Session configuration options
		* @param AbstractProxy\|\SessionHandlerInterface\|null $handler

Copy link

Contributor

TobionSep 25, 2017

Choose a reason for hiding this comment

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

same, AbstractProxy should not be documented anymore

Remove deprecated classes from phpdoc; revert test changes

fdfe60b

Copy link

Member

nicolas-grekas commentedSep 26, 2017

@Tobion OK for you? Can I let you merge if yes?

Copy link

Contributor

Tobion commentedSep 26, 2017

Thank you@afurculita.

Tobion added a commit that referenced this pull request

Sep 26, 2017

feature#24239[HttpFoundation] Deprecate compatibility with PHP <5.4…

bc84304

… sessions (afurculita)This PR was squashed before being merged into the 3.4 branch (closes#24239).Discussion----------[HttpFoundation] Deprecate compatibility with PHP <5.4 sessions| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -This PR removes functionality added in Symfony 2.1 as a compatibility layer with sessions from PHP <5.4.- [x] Fix testsCommits-------3deb394 [HttpFoundation] Deprecate compatibility with PHP <5.4 sessions

Tobion closed this

Sep 26, 2017

afurculita deleted the 3.4 branch

September 26, 2017 12:26

afurculita restored the 3.4 branch

September 26, 2017 12:26

Copy link

Contributor

Tobion commentedSep 26, 2017

@afurculita could you work on a PR against master to remove the deprecated stuff? Would be much appreciated.

Copy link

ContributorAuthor

afurculita commentedSep 26, 2017

@Tobion already started it ;)

afurculita mentioned this pull request

Sep 26, 2017

[HttpFoundation] Removed compatibility layer for PHP <5.4 sessions#24338

Merged

fabpot added a commit that referenced this pull request

Sep 27, 2017

feature#24338[HttpFoundation] Removed compatibility layer for PHP <…

e3b000d

…5.4 sessions (afurculita)This PR was merged into the 4.0-dev branch.Discussion----------[HttpFoundation] Removed compatibility layer for PHP <5.4 sessions| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?  | yes| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -This is a follow-up of#24239. This PR removes the compatibility layer added for sessions for PHP <5.4.Commits-------37d1a21 Removed compatibility layer for PHP <5.4 sessions

This was referencedOct 18, 2017

Release v3.4.0-BETA1#24609

Merged

Release v4.0.0-BETA1#24612

Merged