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

feat: Change session manipulation methods from private to protected #1113

Merged
merged 2 commits into from
Jun 8, 2024

Conversation

michalsn
Copy link
Member

@michalsn michalsn commented May 3, 2024

Description
This PR changes the session manipulation methods from private to protected, so we can use them if we extend the Authentication\Authenticators\Session class.

Reference: #1111

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis
Copy link
Member

kenjis commented May 6, 2024

This Session class is already very long, and would violate the SRP.
Don't we need a new class for these methods?

@michalsn
Copy link
Member Author

michalsn commented May 6, 2024

The methods related to the user session will give us about 50 lines of code. I don't see the point of moving them to a separate class - especially if they are to remain non-public.

@kenjis
Copy link
Member

kenjis commented May 13, 2024

The session data manipulation is needed only for the Session authenticator.
So extracting them into a class does not seem to help much in the future.
If we extract classes, we should extract another functions, but so far there is no need.

By the way, is this an enhancement or a bug fix?
In other words, should the next release be minor version up?

@michalsn
Copy link
Member Author

I would say this is an enhancement.

@michalsn michalsn changed the title Change session manipulation methods from private to protected feat: Change session manipulation methods from private to protected May 13, 2024
@kenjis kenjis added the enhancement New feature or request label May 13, 2024
@kenjis
Copy link
Member

kenjis commented May 13, 2024

Then it should be released as v1.1.0.

@michalsn
Copy link
Member Author

Okay. I'm not familiar with the Shield release cycle, so it's up to you when you'd like to merge this.

@kenjis kenjis merged commit aa40f9c into codeigniter4:develop Jun 8, 2024
37 checks passed
@kenjis kenjis mentioned this pull request Jun 13, 2024
5 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants