Skip to content

[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.

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
Dec 17, 2023
Merged

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

merged 1 commit into from
Dec 17, 2023

Conversation

Florian-Merle
Copy link
Contributor

@Florian-Merle Florian-Merle commented Dec 13, 2023 •
edited
Loading

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

The AbstractSessionListener being marked as internal, its public constant NO_AUTO_CACHE_CONTROL_HEADER should not be used while the documentation states 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 the AbstractSessionListener internal but this means the class could be extended.
Also, maybe the class AbstractSessionListener 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 the AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER const just like the documentation says we can.

@carsonbot carsonbot added this to the 7.1 milestone Dec 13, 2023
@carsonbot
Copy link

Hey!

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

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://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

@chalasr
Copy link
Member

chalasr commented Dec 13, 2023 •
edited
Loading

I suggest to make AbstractSessionListener @final instead of @internal remove the @internal flag from the abstract class and 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.

@stof
Copy link
Member

stof commented Dec 13, 2023

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

@chalasr
Copy link
Member

chalasr commented Dec 13, 2023

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

@Florian-Merle Florian-Merle force-pushed the move-no-auto-cache-control-header-constant branch from 733f4c4 to dcf62d8 Compare December 14, 2023 13:10
@Florian-Merle Florian-Merle changed the base branch from 7.1 to 5.4 December 14, 2023 13:10
@Florian-Merle 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 chalasr modified the milestones: 7.1, 5.4 Dec 14, 2023
chalasr
chalasr reviewed Dec 15, 2023
View reviewed changes
src/Symfony/Component/HttpKernel/CHANGELOG.md Outdated
@@ -9,6 +9,7 @@ CHANGELOG
* 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

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
chalasr approved these changes Dec 15, 2023
View reviewed changes
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.

with minor comment

@OskarStark OskarStark changed the title [HttpKernel] Move @internal from AbstractSessionListener class to its methods and properties [HttpKernel] Move @internal from AbstractSessionListener class to its methods and properties Dec 16, 2023
@fabpot fabpot force-pushed the move-no-auto-cache-control-header-constant branch from dcf62d8 to defe229 Compare December 17, 2023 19:08
@fabpot
Copy link
Member

fabpot commented Dec 17, 2023

Thank you @Florian-Merle.

@fabpot fabpot merged commit 1de8768 into symfony:5.4 Dec 17, 2023
@Florian-Merle Florian-Merle deleted the move-no-auto-cache-control-header-constant branch December 18, 2023 15:26
This was referenced Dec 30, 2023
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@chalasr chalasr chalasr approved these changes

@dunglas dunglas Awaiting requested review from dunglas

@OskarStark OskarStark Awaiting requested review from OskarStark

Assignees
No one assigned
Labels
Bug HttpKernel Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
5 participants
@Florian-Merle @carsonbot @chalasr @stof @fabpot