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

Session Config #5021

Closed
Closed

Conversation

mostafakhudair
Copy link
Contributor

Split session configuration from Config::App to a new config class Config::Session

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

MGatner and others added 30 commits June 8, 2021 16:11
Co-authored-by: Mostafa Khudair <59371810+mostafakhudair@users.noreply.github.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: John Paul E. Balandan, CPA <51850998+paulbalandan@users.noreply.github.com>
paulbalandan and others added 2 commits August 23, 2021 17:45
…reign-key

Fix bug with DB-Forge composite foreign keys
…onfig

Add missing generators' config view file
@mostafakhudair mostafakhudair force-pushed the session-config branch 2 times, most recently from c64e7c0 to a6e1e6a Compare August 24, 2021 14:26
@mostafakhudair
Copy link
Contributor Author

unfamiliar fail

@MGatner
Copy link
Member

MGatner commented Aug 25, 2021

I've always wondered why this wasn't its own thing. Thanks for the work on this! I can't look in depth right now, but the failure is unrelated - I need to figure out how to make our architectural analysis installer version-independent.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Still pondering this. My hunch is we will need to adjust the priority for the Config files. More later.

*
* @var bool
*/
protected $matchIP = false;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed? Is this duplicated by BaseHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is


============================== ============================================ ================================================= ============================================================================================
Preference Default Options Description
============================== ============================================ ================================================= ============================================================================================
**sessionDriver** CodeIgniter\\Session\\Handlers\\FileHandler CodeIgniter\\Session\\Handlers\\FileHandler The session storage driver to use.
**handler** CodeIgniter\\Session\\Handlers\\FileHandler CodeIgniter\\Session\\Handlers\\FileHandler The session storage driver to use.
Copy link
Member

Choose a reason for hiding this comment

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

Table is malformed maybe due to spacing.

*
* @var int
*/
protected $lifetime = 7200;
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of replicating across all other handlers you can define this instead in BaseHandler. Even if there are handlers not using this, at least it is there available.

*
* @var string
*/
protected $handler = FileHandler::class;
Copy link
Member

Choose a reason for hiding this comment

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

I would be in favor of marking these new props as private to reduce BC concerns in the future. This also promotes encapsulation. If a user will find this implementation as lacking, they can abstract one using the SessionInterface instead of extending this and complaining this and that. But this is just my preference.

Copy link
Member

Choose a reason for hiding this comment

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

I understand your preference.

But the way to extend the core classes is described in the user guide.
https://codeigniter4.github.io/CodeIgniter4/extending/core_classes.html#extending-core-classes
And CI users have been using it a lot for a long time.

Making properties as private is big change. I don't think it's easy to decide.

@MGatner
Copy link
Member

MGatner commented Sep 8, 2021

@mostafakhudair We would like to do another release soon, and I would love to see this as part of it. Could you address Paul's requests? I will make it a point to do my full review soon.

@kenjis
Copy link
Member

kenjis commented Sep 8, 2021

It seems the base branch of this PR must be changed to develop.

@kenjis
Copy link
Member

kenjis commented Sep 9, 2021

@MGatner What is the version number of the next release? 4.1.5? 4.2.0?

@mostafakhudair
Copy link
Contributor Author

@MGatner It will be ready tomorrow

@mostafakhudair
Copy link
Contributor Author

@MGatner What is the version number of the next release? 4.1.5? 4.2.0?

@MGatner should this go with develop or stick with 4.2 branch

* Set to `0` means expire when the browser is closed.
=======
* Setting to 0 (zero) means expire when the browser is closed.
>>>>>>> 8d8c82bba537670571013bfdbd155c7a4e91b2aa
Copy link
Member

Choose a reason for hiding this comment

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

There is a conflict.

@MGatner
Copy link
Member

MGatner commented Sep 12, 2021

@mostafakhudair Please redirect to develop and rebase. Sorry for the bouncing back and forth - this should never happen again, was an odd artifact of restyling the codebase.

@kenjis Next version will be 4.1.5, including everything that was on the 4.2 branch (minus Publisher), and releasing as soon as this PR is ready 😉

kenjis added a commit to kenjis/CodeIgniter4 that referenced this pull request Sep 12, 2021
@kenjis kenjis mentioned this pull request Sep 12, 2021
2 tasks
@paulbalandan paulbalandan changed the base branch from 4.2 to develop September 12, 2021 13:16
@mostafakhudair mostafakhudair deleted the session-config branch September 14, 2021 18:21
@mostafakhudair mostafakhudair restored the session-config branch September 14, 2021 18:25
@MGatner
Copy link
Member

MGatner commented Sep 14, 2021

@mostafakhudair Are we getting this back? Maybe in a fresh PR? Keen on this.

@mostafakhudair mostafakhudair deleted the session-config branch September 14, 2021 20:00
@mostafakhudair mostafakhudair mentioned this pull request Sep 14, 2021
5 tasks
@mostafakhudair
Copy link
Contributor Author

@MGatner new clean PR created #5088

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet