Skip to content

docs: improve Session::destroy()#7505

Merged
kenjis merged 3 commits intocodeigniter4:developfrom
kenjis:fix-docs-session-destroy
Jul 3, 2023
Merged

docs: improve Session::destroy()#7505
kenjis merged 3 commits intocodeigniter4:developfrom
kenjis:fix-docs-session-destroy

Conversation

@kenjis
Copy link
Copy Markdown
Member

@kenjis kenjis commented May 17, 2023

Description
Not recomment to use PHP function.
It is a bad practice.

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 kenjis added the documentation Pull requests for documentation only label May 17, 2023
.. literalinclude:: sessions/037.php

This method will work in exactly the same way as PHP's
`session_destroy() <https://www.php.net/session_destroy>`_ function.
Copy link
Copy Markdown
Member Author

@kenjis kenjis May 17, 2023

Choose a reason for hiding this comment

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

The method just calls session_destroy(), so this is correct.

But when using the Session library, a cookie that removes the session cookie will be issued if you call Session::destory() or session_destroy().
This behavior is different from PHP's session_start() and session_destroy().

I don't know why the cookie is issued.


.. note:: This must be the last session-related operation that you do
during the same request. All session data (including flashdata and
tempdata) will be destroyed permanently and functions will be
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

session_destroy() does not unset any of the global variables associated with the session.
https://www.php.net/manual/en/function.session-destroy.php#refsect1-function.session-destroy-description

So we can still get the session data after calling Session::destory().

    public function destroy()
    {
        $session = session();
        $session->destroy();

        d($session->get(), $_SESSION);
        $session->set('foo', 'bar');
        d($session->get(), $_SESSION);
    }
/Users/kenji/work/codeigniter/official/CodeIgniter4/app/Controllers/Home.php:20:
array (size=1)
  '__ci_last_regenerate' => int 1684292786

/Users/kenji/work/codeigniter/official/CodeIgniter4/app/Controllers/Home.php:20:
array (size=1)
  '__ci_last_regenerate' => int 1684292786

/Users/kenji/work/codeigniter/official/CodeIgniter4/app/Controllers/Home.php:22:
array (size=2)
  '__ci_last_regenerate' => int 1684292786
  'foo' => string 'bar' (length=3)

/Users/kenji/work/codeigniter/official/CodeIgniter4/app/Controllers/Home.php:22:
array (size=2)
  '__ci_last_regenerate' => int 1684292786
  'foo' => string 'bar' (length=3)

Is this intended behavior or a bug?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@codeigniter4/core-team Any opinion?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm... since we set the session.use_strict_mode, the new cookie shouldn't be needed, but I'm wondering about the cases where the hosting provider disabled ini_set() for security reasons.

There are definitely differences between the native session_destroy() and our implementation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is a "bug", but it might be unexpected behavior for some. Since this PR is just an update to the docs I don't think we worry about it, except maybe to add a note of caution.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@michalsn We use ini_set() outside of the Session.

BaseHandler.php
        ini_set('session.save_path', $this->savePath);
bootstrap.php
ini_set('display_errors', '1');
ini_set('display_startup_errors', '1');
CodeIgniter.php
     * not complain when ini_set() function is used.
Exceptions.php
            ini_set('highlight.comment', '#767a7e; font-style: italic');
            ini_set('highlight.default', '#c7c7c7');
            ini_set('highlight.html', '#06B');
            ini_set('highlight.keyword', '#f1ce61;');
            ini_set('highlight.string', '#869d6a');
FileHandler.php
            ini_set('session.save_path', $this->savePath);
            ini_set('session.sid_length', (string) $SIDLength);
MemcachedHandler.php
            ini_set('memcached.sess_prefix', $this->keyPrefix);
Session.php
            ini_set('session.name', $this->sessionCookieName);
        ini_set('session.cookie_samesite', $sameSite);
            ini_set('session.gc_maxlifetime', (string) $this->sessionExpiration);
            ini_set('session.save_path', $this->sessionSavePath);
        ini_set('session.use_trans_sid', '0');
        ini_set('session.use_strict_mode', '1');
        ini_set('session.use_cookies', '1');
        ini_set('session.use_only_cookies', '1');
            ini_set('session.sid_length', (string) $sidLength);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@MGatner Okay, this is not a bug.
I updated the description.

@paulbalandan paulbalandan changed the title docs: improve Session::destory() docs: improve Session::destroy() Jun 23, 2023
@kenjis kenjis force-pushed the fix-docs-session-destroy branch from c9d0ebd to cba7713 Compare July 3, 2023 02:05
@kenjis kenjis requested review from MGatner and michalsn July 3, 2023 02:14
@kenjis
Copy link
Copy Markdown
Member Author

kenjis commented Jul 3, 2023

@kenjis kenjis merged commit 8bcc33e into codeigniter4:develop Jul 3, 2023
@kenjis kenjis deleted the fix-docs-session-destroy branch July 3, 2023 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Pull requests for documentation only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants