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

[HttpKernel] Move`@internal` from`AbstractSessionListener` class to its methods and properties#53057

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

Merged

fabpot merged 1 commit intosymfony:5.4fromFlorian-Merle:move-no-auto-cache-control-header-constant

Dec 17, 2023

Merged

[HttpKernel] Move`@internal` from`AbstractSessionListener` class to its methods and properties#53057

fabpot merged 1 commit intosymfony:5.4fromFlorian-Merle:move-no-auto-cache-control-header-constant

Dec 17, 2023

Conversation

Copy link

Contributor

Florian-Merle commentedDec 13, 2023•
edited
Loading

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues
License	MIT

TheAbstractSessionListener being marked as internal, its public constantNO_AUTO_CACHE_CONTROL_HEADER should not be used while the documentationstates it can.

In fact, static analysis tools like psalm says there is an error with code using this constant.

ERROR: InternalClass - xxx.php:32:33 - Symfony\Component\HttpKernel\EventListener\AbstractSessionListener is internal to Symfony but called from xxx (see https://psalm.dev/174)        $response->headers->set(AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER, 'true');

~~Another solution is to make every method of theAbstractSessionListener internal but this means the class could be extended.~~
~~Also, maybe the classAbstractSessionListener should not be internal, but I don't think so.~~
~~This is why I introduced a new interface that is not internal and allows to not introduce BC.~~

~~The documentation will need to be updated if this pull request is merged, I'd be happy to do it later.~~

As discussed, I made public/protected properties and methods internal and removed the original internal mark on the class.
This solves the issue and allows us to use theAbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER const just like the documentation says we can.

carsonbot added Status: Needs Review Bug HttpKernel labels

Dec 13, 2023

carsonbot added this to the7.1 milestone

Dec 13, 2023

Copy link

carsonbot commentedDec 13, 2023

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

Copy link

Member

chalasr commentedDec 13, 2023•
edited
Loading

I suggest to~~makeAbstractSessionListener@final instead of@internal~~ remove the@internal flag from the abstract classand mark all of its methods as@internal instead, so we don't have to introduce an interface for a single constant and this PR can target 5.4 as a bugfix.

Copy link

Member

stof commentedDec 13, 2023

an abstract class being defined as@final does not make any sense.

Copy link

Member

chalasr commentedDec 13, 2023

Right, stoffed again 😅
Proposal updated to just mark its methods as internal.

Florian-Merle force-pushed themove-no-auto-cache-control-header-constant branch from733f4c4 todcf62d8Compare

December 14, 2023 13:10

Florian-Merle requested review fromOskarStark anddunglas ascode owners

December 14, 2023 13:10

Florian-Merle changed the base branch from7.1 to5.4

December 14, 2023 13:10

Florian-Merle changed the title~~[HttpKernel] Move const NO_AUTO_CACHE_CONTROL_HEADER to new interface…~~[HttpKernel] Move @internal from AbstractSessionListener class to its methods and properties

Dec 14, 2023

chalasr modified the milestones:7.1,5.4

Dec 14, 2023

chalasr reviewed

Dec 15, 2023

View reviewed changes

src/Symfony/Component/HttpKernel/CHANGELOG.md Outdated

		* Deprecate the`fileLinkFormat` parameter of`DebugHandlersListener`
		* Add support for configuring log level, and status code by exception class
		* Allow ignoring "kernel.reset" methods that don't exist with "on_invalid" attribute
		* Un-mark`AbstractSessionListener` as internal and mark all of its public/protected method/properties as internal

Copy link

Member

chalasrDec 15, 2023

Choose a reason for hiding this comment

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

Should be reverted as changelogs are auto-generated for bugfix releases

chalasr approved these changes

Dec 15, 2023

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

with minor comment

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Dec 15, 2023

OskarStark changed the title~~[HttpKernel] Move @internal from AbstractSessionListener class to its methods and properties~~[HttpKernel] Move@internal fromAbstractSessionListener class to its methods and properties

Dec 16, 2023