Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some sf2 references #23936

Merged
merged 1 commit into from Aug 22, 2017
Merged

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Aug 20, 2017

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

Those sf2 references can be safely remove and changed to symfony as there are internal or they don't break anything when upgrading Symfony's version:

  • Flashes are ephemeral, so changing the key in the session is not a problem;
  • Changing the local storage key for the profiler is not an issue as this is a dev tool.

@fabpot
Copy link
Member Author

fabpot commented Aug 20, 2017

I think changing the key for sessions is also possible. Session would be "lost" when upgrading, or we could add some code to move data from the old key to the new one whenever we read some session data. WDYT?

@linaori
Copy link
Contributor

linaori commented Aug 20, 2017

I think there should be a "BC" layer for the session for sure, due to e-commerce applications using the session. Other than that, defo 👍

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

OK for me as is, flashes are short lived enough to me to not bother adding a BC layer.

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Tests need an update (testGetStorageKey)

@dmaicher
Copy link
Contributor

dmaicher commented Aug 21, 2017

I would also suggest changing the session attributes bag key _sf2_attributes (https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/HttpFoundation/Session/Attribute/AttributeBag.php#L36) should have a BC layer then. So in 3.4 we would read from the old key as well and in 4.0 only from the new key? Obviously upgrading from 3.3 directly to 4.0 would then not have the BC layer 😄 That should be documented probably in the UPGRADE-4.0.md so that people first upgrade to 3.4 if the sessions are critical?

Edit: Or can we just not have a BC layer and ask people to "overwrite" the session service with an AttributeBag that uses the old key in case its really critical for them?

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 22, 2017
@fabpot fabpot changed the base branch from master to 3.4 August 22, 2017 21:10
@fabpot
Copy link
Member Author

fabpot commented Aug 22, 2017

I will let someone else remove the remaining sf2 in sessions, with the appropriate BC layer. I would keep the BC layer in 4.0 as well. Just copy from the old key to the new one silently.

@fabpot fabpot merged commit 1b02654 into symfony:3.4 Aug 22, 2017
fabpot added a commit that referenced this pull request Aug 22, 2017
This PR was merged into the 3.4 branch.

Discussion
----------

Remove some sf2 references

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

Those `sf2` references can be safely remove and changed to `symfony` as there are internal or they don't break anything when upgrading Symfony's version:

* Flashes are ephemeral, so changing the key in the session is not a problem;
* Changing the local storage key for the profiler is not an issue as this is a dev tool.

Commits
-------

1b02654 removed sf2 references
@fabpot fabpot deleted the sf2-references-removal branch September 11, 2017 18:18
This was referenced Oct 18, 2017
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
This PR was merged into the 3.4 branch.

Discussion
----------

Remove some sf2 references

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

Those `sf2` references can be safely remove and changed to `symfony` as there are internal or they don't break anything when upgrading Symfony's version:

* Flashes are ephemeral, so changing the key in the session is not a problem;
* Changing the local storage key for the profiler is not an issue as this is a dev tool.

Commits
-------

1b02654 removed sf2 references
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants