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

auto session start #1097

Merged
merged 6 commits into from
Jul 27, 2018
Merged

auto session start #1097

merged 6 commits into from
Jul 27, 2018

Conversation

bangbangda
Copy link
Contributor

Start automatically when the session is initialized.

Why is it a separate $session->start() ?

Before:
$session = session()->start();
After:
$session = session();

Before:
$session = \Config\Services::session($config);
$session->start();
After:
$session = \Config\Services::session($config);

Start automatically when the session is initialized.
Why is it a separate $session->start()?
auto start  is not ok?

Before:
$session = session()->start();
After:
$session = session();

Before:
$session = \Config\Services::session($config);
$session->start();
After:
$session = \Config\Services::session($config);
Start automatically when the session is initialized.
@bangbangda bangbangda closed this Jul 9, 2018
@bangbangda bangbangda reopened this Jul 10, 2018
@lonnieezell
Copy link
Member

I wasn't involved in the original architecture of the session stuff, I just ported it over. Looking over the main session class I can't see why we shouldn't do this. However, you should put a check within the start() method to check if it's running and exit the method at the very beginning of it so that we don't try to start again.

@bangbangda
Copy link
Contributor Author

if sessions are enabled, but none exists. start() method will be executed.
so. will not repeat start().

Please confirm the system/Config/Services.php file for details.

@lonnieezell
Copy link
Member

if sessions are enabled, but none exists. start() method will be executed.
so. will not repeat start().

True - assuming everyone plays by the rules. :) Someone can still call start() if they didn't bother to read the manual closely enough. Also, there are a number of people already building sites on CI4 so it's a simple solution to safeguard their sites a little bit more.

@bangbangda
Copy link
Contributor Author

OK.I understand.
Update the code this afternoon.

Start automatically when the session is initialized.
@bangbangda
Copy link
Contributor Author

@lonnieezell
Code update is complete.

https://bcit-ci.github.io/CodeIgniter4/database/model.html#deleting-data

An array of primary keys can be passed in as the first parameter to delete multiple records at once:
`$userModel->delete([1,2,3]);`
@bangbangda
Copy link
Contributor Author

@lonnieezell

@bangbangda
Copy link
Contributor Author

@lonnieezell
Hi. is there a Issues with this PR?

@jim-parry
Copy link
Contributor

@bangbangda Your PR has conflicts that need to be resolved, before travis-ci can be run, and without considering whether or not it is a good idea to auto-start sessions.

@lonnieezell
Copy link
Member

@bangbangda You merged in changes to the Model class that are already in core. Those should not be part of this, please remove.

@bangbangda
Copy link
Contributor Author

@lonnieezell yes.deletion is complete.

@lonnieezell
Copy link
Member

Thanks works for me, Thanks @bangbangda

@lonnieezell lonnieezell merged commit 0d2b287 into codeigniter4:develop Jul 27, 2018
@lonnieezell
Copy link
Member

Although I just realized this might wreak havoc on testing... but we'll work that out since session tests already broken.

@jim-parry
Copy link
Contributor

Hmmm - I remember a similar PR for CI3, and Andrey argued strongly against auto-starting sessions. Don't remember the reasoning.
I don't know that we want to force sessions on evryone, who will then hve to configure them and deal with the hundreds (or much more) of session files resulting from poor or no garbage collection.

I haven't taken a look at the PR, but I trust that any auto-starting is a configurable option.

@lonnieezell
Copy link
Member

From what I recall of Andrey's reasoning - autostarting the session on every page load could lead to DOS attacks from the sheer number of session being created and some other related security type issues. Completely agree with those.

What this PR does, though, is to start the session the first time you request it, instead of asking you to do session->start() immediately afterword. I think it should be fine, as I can't think of any additional actions you'd take with the session library before it's started, and none of the methods in the Session class seemed to provide any benefit prior.

Unless you can think of any issues with that?

@jim-parry
Copy link
Contributor

jim-parry commented Jul 28, 2018 via email

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.

3 participants