Skip to content

Remove some sf2 references #23936

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 22, 2017
Merged

Remove some sf2 references #23936

merged 1 commit into from
Aug 22, 2017

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 👍

nicolas-grekas
nicolas-grekas approved these changes Aug 21, 2017
View reviewed changes
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.

chalasr
chalasr approved these changes Aug 21, 2017
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment •
edited
Loading

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 •
edited
Loading

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 force-pushed the sf2-references-removal branch from 5017a85 to 1b02654 Compare August 22, 2017 20:48
@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
@fabpot
feature #23936 Remove some sf2 references (fabpot)
e8bb73c 
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
Merged
Merged
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
@fabpot
feature symfony#23936 Remove some sf2 references (fabpot)
bec9a48 
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
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
6 participants
@fabpot @linaori @dmaicher @nicolas-grekas @chalasr @carsonbot