Document the session subsystem

Thanks for the patch! I went through and did a review, and it mostly looks pretty good. A few formatting, housekeeping, and typographical things to address:

1. +++ b/core/modules/system/core.api.php
   @@ -973,11 +974,11 @@
   + *   accessed via the requests <code>getSession()

   method in Drupal, which
   

   requests -> request's, or maybe it should be request object's

   Probably it would be good to give a class/interface here for the request too though, or tell how to get hold of the request object?

   Also do not use <code> tags in API docs.

2. +++ b/core/modules/system/core.api.php
   @@ -973,11 +974,11 @@
   + *   <code>\Symfony\Component\HttpFoundation\Session\SessionInterface

   .
   

   Remove code tag

3. +++ b/core/modules/system/core.api.php
   index 0000000..e43cefb
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/system/session.api.php
   
   +++ b/core/modules/system/session.api.php
   +++ b/core/modules/system/session.api.php
   @@ -0,0 +1,83 @@
   

   We do not want any more api.php files added to the core/modules/system directory. We're moving them in another issue. Please put this somewhere else -- somewhere in core/lib/Drupal that has related classes.

   Actually though, since this is just a single defgroup doc block, just put it into the existing core.api.php file rather than creating a whole new file. We only want whole new files when there is a collection of hooks and docs.

4. +++ b/core/modules/system/session.api.php
   @@ -0,0 +1,83 @@
   + * Store and retrieve data associated with a users browsing session.
   

   users -> user's

5. +++ b/core/modules/system/session.api.php
   @@ -0,0 +1,83 @@
   + * @section sec_intro Overview
   + *
   + * The Drupal session management subsystem is built on top of the Symfony
   + * session component. It is optimized in order to minimize the impact of
   

   No blank line here.

6. +++ b/core/modules/system/session.api.php
   @@ -0,0 +1,83 @@
   + * becomes empty. For this reason it is important for contrib and custom code to
   

   contrib -> contributed

7. +++ b/core/modules/system/session.api.php
   @@ -0,0 +1,83 @@
   + * @section sec_usage Usage
   + *
   + * Session data is accessed via the requests <code>getSession()

   method in
   + * Drupal, which returns an instance of
   

   No blank line here.

   Also change
   request -> request object's

   And it would be useful to say what class the request is and how to get it.

8. +++ b/core/modules/system/session.api.php
   @@ -0,0 +1,83 @@
   + * Session data is accessed via the requests <code>getSession()

   method in
   + * Drupal, which returns an instance of
   + * \Symfony\Component\HttpFoundation\Session\SessionInterface. The
   

   Take out "in Drupal".

   No CODE tags.

9. +++ b/core/modules/system/session.api.php
   @@ -0,0 +1,83 @@
   + * most important methods are <code>set()

   , get() and
   + * remove().
   + *
   

   Are these methods on SessionInterface? Since both the request object and the session object are mentioned, this is slightly ambiguous.

   Also no CODE tags.

10. +++ b/core/modules/system/session.api.php
    @@ -0,0 +1,83 @@
    + * relying on the session.
    + *
    + * @code
    + * public function counter(Request $request) {
    

    End previous line with : and remove blank line.

    What is a "counter controller"? Where would this go?

11. +++ b/core/modules/system/session.api.php
    @@ -0,0 +1,83 @@
    + *     '#markup' => $this->t('Number of times you've viewed this page: @count', ['count' => @count]),
    

    Consider shortening this so it fits closer to 80 characters. Maybe make the string:

    "Page views: @count"

    or something like that, because it would not detract from the example I think?

12. +++ b/core/modules/system/session.api.php
    @@ -0,0 +1,83 @@
    + * request. But also because third party session storage backend do not
    + * necessarely allow objects of unlimited size. If it is necessary to collect a
    + * non-trivial amount of data specific to a users session, then use state and
    

    necessarily is misspelled

    Also this is not a sentence. Probably should be combined with previous sentence.

    I think backend should be backends also?

    and
    users => user's

    Would be useful to have a link to the State topic here too?

13. +++ b/core/modules/system/session.api.php
    @@ -0,0 +1,83 @@
    + *
    + *
    + * @section sec_reserved Reserved attributes and namespacing
    + *
    

    Too many blank lines.

14. +++ b/core/modules/system/session.api.php
    @@ -0,0 +1,83 @@
    + * @section sec_reserved Reserved attributes and namespacing
    + *
    + * Contrib modules relying on the session are encouraged to namespace session
    + * attributes by prefixing them with their project name or an abbreviation
    

    No blank line here

    And
    contrib -> contributed

15. +++ b/core/modules/system/session.api.php
    @@ -0,0 +1,83 @@
    + * Some attributes are reserved for Drupal core and must not be accessed from
    + * within contrib and custom code. Reserved attributes include:
    + * - uid: The user ID if a user has been logged in via login form and
    + *   authenticated by the cookie authentication provider. Neither third party
    + *   authentication providers nor unrelated code may change the value of this
    + *   attribute.
    + *
    

    The docs say contrib code cannot *access* this information, but then in the paragraph below it just says you cannot *change* the information. Which is correct?

16. +++ b/core/modules/system/session.api.php
    @@ -0,0 +1,83 @@
    + * within contrib and custom code. Reserved attributes include:
    

    contrib -> contributed

17. +++ b/core/modules/system/session.api.php
    @@ -0,0 +1,83 @@
    + * @section sec_compatibility Backwards compatibility
    + *
    + * Drupal still supports code which accesses the <code>$_SESSION

    + * superglobal directly. However, it is recommended to migrate any such code to
    

    No blank line, no CODE tags.

18. +++ b/core/modules/system/session.api.php
    @@ -0,0 +1,83 @@
    + * @section sec_compatibility Backwards compatibility
    + *
    + * Drupal still supports code which accesses the <code>$_SESSION

    + * superglobal directly. However, it is recommended to migrate any such code to
    + * the new API.
    + *
    

    Actually I think this whole sections should be removed.

I am at Drupalconla, and I am working on this issue.

This patch incorporates comments in #3

@jhodgdon couldn't understand your last comment

Actually I think this whole sections should be removed.

You mean all the sections or the backward compatibility section?

Removed trailing spaces.

+++ b/core/modules/system/core.api.php
@@ -2228,3 +2228,72 @@ function hook_config_schema_info_alter(&$definitions) {
+ * becomes empty. For this reason it is important for contributed and custom code to
...
+ * party session storage backends do not necessarily allow objects of unlimited size.
+ * If it is necessary to collect a non-trivial amount of data specific to a user's
...
+ * Contributed modules relying on the session are encouraged to namespace session

Those lines should be wrapped at 80 cols.

In item 10 of comment #3, I meant that we should not be talking about backwards compatibility -- remove this section entirely. We just don't want to talk about non-recommended APIs in our API docs.

Updated patch for documentation

• Removed Backward compatibility section
• Wrapped overflowing words to 80 cols

Thanks for the new patch! A few things still need to be fixed:

1. +++ b/core/modules/system/core.api.php
   @@ -973,11 +973,11 @@
   + *   it is not stored the same way so it's a separate type here. Session data is
   + *   accessed via the Symfony\Component\HttpFoundation\Request::getSession(), which
   + *   returns an instance of
   

   This second line is now over 80 characters long, so I think this needs a little more wrapping attention. Same for some of the lines farther down... I'll mark them.

   Also omit "the" in this second line.

2. +++ b/core/modules/system/core.api.php
   @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
   + * becomes empty. For this reason it is important for contributed and custom code to
   

   over 80 chars

3. +++ b/core/modules/system/core.api.php
   @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
   + * Session data is accessed via the Symfony\Component\HttpFoundation\Request::getSession()
   

   over 80

4. +++ b/core/modules/system/core.api.php
   @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
   + * \Symfony\Component\HttpFoundation\Session\SessionInterface. The
   + * most important methods on SessionInterface are set(), get() and
   + * remove().
   + *
   

   these are pretty far under 80 chars and should be rewrapped too.

5. +++ b/core/modules/system/core.api.php
   @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
   + * public function reset(Request $request) {
   + *   $session = $request->getSession();
   + *   $session->remove('mymodule.counter');
   + * }
   + * @endcode
   

   This is interesting, but how/when/why would this reset() method get called? It's not clear from the code fragment at all, to me at least.

6. +++ b/core/modules/system/core.api.php
   @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
   + * minimum as the complete session is loaded on every request. Also third
   

   Probably could use a comma before "as".

7. +++ b/core/modules/system/core.api.php
   @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
   + * party session storage backends do not necessarily allow objects of unlimited ¶
   + * size. If it is necessary to collect a non-trivial amount of data specific to ¶
   

   Trailing spaces on the first two lines.

8. +++ b/core/modules/system/core.api.php
   @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
   + * a user's session, then use @link state state topic @endlink and store the key
   + * to the state-entry in the session.
   

   The link text doesn't make sense in the sentence, which would read "... use state topic and store the key ...". So that needs to be fixed. Probably best to make a plain sentence here, then say "See the (link)State topic(endlink) for more information."

   Also, I don't think state-entry should be hyphenated.

   And maybe this should say "only store the key to the state entry"? [add "only"]?

9. +++ b/core/modules/system/core.api.php
   @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
   + * Contributed modules relying on the session are encouraged to namespace session
   

   over 80 chars

10. +++ b/core/modules/system/core.api.php
    @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
    + * Some attributes are reserved for Drupal core and must not be accessed from
    

    I think I may have mentioned this before, but this says "must not be accessed", when I think it just means it can't be *changed*, right?

11. +++ b/core/modules/system/core.api.php
    @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
    + *   authenticated by the cookie authentication provider. Neither third party
    

    third-party should be hyphenated

12. +++ b/core/modules/system/core.api.php
    @@ -2229,3 +2229,68 @@ function hook_config_schema_info_alter(&$definitions) {
    + *   authentication providers nor unrelated code may change the value of this
    

    what is "unrelated code"? Maybe this should actually just say that the value cannot be modified and not specify who cannot modify it?

Thanks for the new patch -- this is looking a lot better!

There are still a few things to address:

1. +++ b/core/modules/system/core.api.php
   @@ -2227,3 +2227,67 @@ function hook_config_schema_info_alter(&$definitions) {
   +
   +
   +/**
   

   Should only be one blank line here, I think, before the new Sessions topic?

2. +++ b/core/modules/system/core.api.php
   @@ -2227,3 +2227,67 @@ function hook_config_schema_info_alter(&$definitions) {
   + * Symfony\Component\HttpFoundation\Request::getSession()
   

   Class namespace needs to start with \ here

3. +++ b/core/modules/system/core.api.php
   @@ -2227,3 +2227,67 @@ function hook_config_schema_info_alter(&$definitions) {
   + * important methods on SessionInterface are set(), get() and remove().
   

   In the Drupal project, we use serial commas.

   So this should be:

   set(), get(), and remove().

4. +++ b/core/modules/system/core.api.php
   @@ -2227,3 +2227,67 @@ function hook_config_schema_info_alter(&$definitions) {
   + *     '#markup' => $this->t('Page Views: @count', ['count' => @count]),
   

   This is a bit odd. Normally, "page views" would be "how many times the public has viewed this page", not "how many pages have I visited on this site".

   Given that this is a contrived example, and that the (fake) UI is confusing, I think we should either:

   a) Explain above the example that it counts the number of pages the user visits (at least, I think that is maybe what it is doing?)

   or

   b) Change this UI line to make that clearer.

   In either case, a bit of explanation about when this controller gets invoked and how would be good. I still do not know what a "counter controller" is, what class it's defined in, how it makes itself known to Drupal, etc. All of that makes this example very confusing. For me, it raises more questions than it answers. If we're going to put in a long example like this, we need to have it be something that really clarifies things.

5. +++ b/core/modules/system/core.api.php
   @@ -2227,3 +2227,67 @@ function hook_config_schema_info_alter(&$definitions) {
   + * minimum, as the complete session is loaded on every request. Also third
   + * party session storage backends do not necessarily allow objects of unlimited
   + * size. If it is necessary to collect a non-trivial amount of data specific to
   

   This sentence could use some punctuation... how about:

   Also, third-party session storage ...

6. +++ b/core/modules/system/core.api.php
   @@ -2227,3 +2227,67 @@ function hook_config_schema_info_alter(&$definitions) {
   + * size. If it is necessary to collect a non-trivial amount of data specific to
   + * a user's session, See the (link)State topic(endlink) for more information
   + * and only store the key to the state entry in the session.
   

   I think someone took one of my previous review comments a bit too literally here -- sorry for being confusing! ;)

   To make a link, see
   https://www.drupal.org/node/1354/#link

   (@link ... @endlink tags)

   Also, ... this seems to be two sentences combined with the second in the middle?

   So I think it should say:

   If it is necessary to collect... specific to a user's session, use the State system and only store the key to the state entry in the session. See the State topic for more information.

   (and turn "State topic" into a link using @link @endlink)

7. +++ b/core/modules/system/core.api.php
   @@ -2227,3 +2227,67 @@ function hook_config_schema_info_alter(&$definitions) {
   + * Some attributes are reserved for Drupal core and must not be accessed from
   + * within contributed and custom code. Reserved attributes include:
   + * - uid: The user ID if a user has been logged in via login form and
   + *   authenticated by the cookie authentication provider. Value of this
   + * attribute cannot be modified.
   + * @}
   

   OK, trying to make this point again... The first sentence says "... must not be accessed...". [This is not accurate, I think, since it can be accessed by reading, right?] The last bit says "... cannot be modified" [this seems more accurate]. But they cannot contradict each other in the same paragraph.

   Also please do not make a bullet list unless there is more than one item in the list. If there is only one item to talk about, just say something like "For example".

   So. All that taken into account, this can be a lot shorter.

   How about:

   Some session attributes put into the session by Drupal core should not be modified by contributed and custom code. For example, the 'uid' attribute, which stores the user ID for a logged-in user, must not be modified.

I think we should have the example in there. I just think it needs more explanation, and if possible it should be shorter.

So. The current patch says:

+ * The following code fragment shows the implementation of a counter controller
+ * relying on the session:
+ * @code
+ * public function counter(Request $request) {
+ *   $session = $request->getSession();
+ *   $count = $session->get('mymodule.counter', 0) + 1;
+ *   $session->set('mymodule.counter', $count);
+ *
+ *   return [
+ *     '#markup' => $this->t('Page Views: @count', ['count' => @count]),
+ *     '#cache' => [
+ *       'max-age' => 0,
+ *     ]
+ *   ];
+ * }
+ *
+ * public function reset(Request $request) {
+ *   $session = $request->getSession();
+ *   $session->remove('mymodule.counter');
+ * }
+ * @endcode

Let's change the preceding sentence to say something like this (assuming this is accurate -- I **still** don't know where this code goes or how it would be called for sure):

The following code fragment, which would be called from the controller for a page to count the number of times the page has been visited by the current user, illustrates how to use session information:

Then put in the counter() method example.

Then for the reset() method, you also need to explain in a similar way where the code would go and how it would be called. I have no idea. I think it's important to put the code in, but it needs an explanation.

Thanks!

Oh, and regarding the explanation for the uid attribute, OK... So maybe we should say that you shouldn't assume that information in the session can be used the way you think you can, and then give that explanation you just gave in comment #14 about the uid? Because that is a great example of why you can't make assumptions.

And then we should probably also say that you should never modify session information that you didn't put in there yourself.

Anot6her thing how to extend section of "uid" if any other core parts will start to store things, like #2430379: Add explicit test for session based language negotiation
So there should be a d.o page about

I moved the patch text to the new location of core.api.php.

I also slightly changed the section on "If it is necessary to collect a non-trivial amount of data", to say this should be stored using the Key/Value store, not the State store, because State is site-specific, not user-session-specific

Improved the text again on using Key / Value instead of State

Let's have some reviews.

Thanks! This is looking much better than the last time I reviewed it. It still needs a small amount of work, I think:

1. The issue summary says "... In some places (like core.api.php) documentation is wrong.". I don't see this patch addressing that problem. Maybe @zenerol can explain what is wrong that needs to be fixed?

2. +++ b/core/core.api.php
   @@ -1144,9 +1144,11 @@
   + *   accessed via Symfony\Component\HttpFoundation\Request::getSession(), which
   

   Start namespaces with \ in docs

3. +++ b/core/core.api.php
   @@ -2419,3 +2421,65 @@ function hook_validation_constraint_alter(array &$definitions) {
   + * and the session cookie is removed from the browser as soon as the session
   + * becomes empty. For this reason it is important for contributed and custom
   

   What does "session becomes empty" mean? It seems to imply that there is no session data; if that is what it means, let's say that instead of saying "session becomes empty".

4. +++ b/core/core.api.php
   @@ -2419,3 +2421,65 @@ function hook_validation_constraint_alter(array &$definitions) {
   + * Symfony\Component\HttpFoundation\Request::getSession()
   

   Namespaces need to start with \ in docs

5. +++ b/core/core.api.php
   @@ -2419,3 +2421,65 @@ function hook_validation_constraint_alter(array &$definitions) {
   + * important methods on SessionInterface are set(), get() and remove().
   

   We use serial commas in Drupal docs, so we need a comma after get() here.

6. +++ b/core/core.api.php
   @@ -2419,3 +2421,65 @@ function hook_validation_constraint_alter(array &$definitions) {
   + *     '#markup' => $this->t('Page Views: @count', ['count' => @count]),
   

   The array at the end of this line should I think be:

   ['@count' => $count]

7. +++ b/core/core.api.php
   @@ -2419,3 +2421,65 @@ function hook_validation_constraint_alter(array &$definitions) {
   + *     ]
   

   Add a comma after ] to conform to our coding standards.

8. +++ b/core/core.api.php
   @@ -2419,3 +2421,65 @@ function hook_validation_constraint_alter(array &$definitions) {
   + * abbreviation thereof.
   

   maybe add

   , to avoid name conflicts.

   at the end?

9. +++ b/core/core.api.php
   @@ -2419,3 +2421,65 @@ function hook_validation_constraint_alter(array &$definitions) {
   + * within contributed and custom code. Reserved attributes include:
   

   Is there only 1 to mention?

10. +++ b/core/core.api.php
    @@ -2419,3 +2421,65 @@ function hook_validation_constraint_alter(array &$definitions) {
    + * - uid: The user ID if a user has been logged in via login form and
    + *   authenticated by the cookie authentication provider. Value of this
    + * attribute cannot be modified.
    

    Last line needs 2 spaces of indentation here.

    Also this section needs some additional a/an/the added to read better.

Hello, I have applied the last patch added by @cpj. Made changes as mentioned by @jhodgdon.
I am not very much clear about the points 1, 9 & 10. So I have covered other points in the patch. Please review this.

Here are some clarifications:

Point 1: If you use a class with a namespace in docs, like \Drupal\Core\Something, you need to start the class/namespace with a \ so not Drupal\Core\Something but \Drupal\Core\Something.

Point 9: Ah, the next line of context should have been included... So the text says "Reserved attributes include" but the next lines only have 1 attribute discussed. So I was just wondering if there were any more? You can ignore this.

Point 10: The grammar is not very good. in these three sentences and the words "a", "an", and "the" need to be added in various places to make it read better. Also all three lines in this area need to have the same indentation.

Also PLEASE make an interdiff file when you upload a new patch and fix things mentioned in a review. It makes it easier for people to see what you have changed. In this case, please fix these points and make an interdiff starting from patch in #20. Thanks! Your patch will not be reviewed without this.

+++ b/core/core.api.php
@@ -2419,3 +2421,65 @@ function hook_validation_constraint_alter(array &$definitions) {
+ *
+ * @section sec_usage Usage
+ * Session data is accessed via the
+ * Symfony\Component\HttpFoundation\Request::getSession()
+ * method, which returns an instance of
+ * \Symfony\Component\HttpFoundation\Session\SessionInterface. The most
+ * important methods on SessionInterface are set(), get(), and remove().
+ *

Symfony\Component\HttpFoundation\Request::getSession() on line 2441 should start with a leading backslash, the way \Symfony\Component\HttpFoundation\Session\SessionInterface does on line 2443.

Here's a rerolled patch with all the points in #26 and #27 taken care of.

+ * - uid: The user ID if a user has been logged in via login form and
+ *   authenticated by the cookie authentication provider. Value of this
+ * attribute cannot be modified.

I simplified this bit, as I don't think we need to explain here how Drupal core handles user login. We can just say the uid of an authenticated user.

> Also PLEASE make an interdiff file when you upload a new patch and fix things mentioned in a review.

Shameless plug: I've released a script that automates making patches and interdiffs: https://github.com/joachim-n/dorgflow

Dear testbot, please test against 8.6

This new documentation is great, and it does apply to 8.6.x. The remarks from #26 and #27 have been resolved and I couldn't find any other things to change.

Nice improvements.

Committed and pushed e00319e280 to 8.6.x and ece8b9a339 to 8.5.x. Thanks!