Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gagarine’s picture

jhodgdon’s picture

Title: Documentation problem with sess_write » sess_write/sess_read documentation should recommend $_SESSION instead

This appears to be a valid point. See the note here:
http://api.drupal.org/api/drupal/includes--session.inc/7
and how this is handled in Drupal 7's internal-use functions:
http://api.drupal.org/api/function/_drupal_session_read/7
as well as the issue linked to above.

So the thing to do would be to add to the docblocks for sess_read() and sess_write() in D6 the same note as is now in Drupal 7:

This function should not be called directly. Session data should instead be accessed via the $_SESSION superglobal.

Actually, it looks like sess_read() and sess_write() don't even have docblocks, so they would need to be written.

jhodgdon’s picture

Issue tags: +Novice

Probably a good project for a novice doc contributor

chx’s picture

sess_read / sess_write is never intended to use at all. I have never ever seen anyone trying to use them in favor $_SESSION.

Edit: and it would not work anyways as these read/write the whole session not on element of it.

gagarine’s picture

Exactly it will not work. But on IRC and forum some people try to use them and lose time... like here http://drupal.org/node/88867

A simple "This function should not be called directly. Session data should instead be accessed via the $_SESSION superglobal." will be perfect.

Jaypan’s picture

sess_read / sess_write is never intended to use at all. I have never ever seen anyone trying to use them in favor $_SESSION.

Edit: and it would not work anyways as these read/write the whole session not on element of it.

That's exactly why you never seen anyone use it, and why the docs should be amended! Most people (myself included) figure this out the hard way.

daniels220’s picture

Assigned: Unassigned » daniels220
Status: Active » Needs review
FileSize
1.11 KB

Here's a patch introducing complete docblocks for the functions. Is my choice of wording at the top good?

jhodgdon’s picture

Status: Needs review » Needs work

I'm not sure about the description of sess_read(). It seems like it does a lot more than just read the session from the database and return it? Maybe both of them need an explantion of what a "session" is? Not sure if this is necessary, because these are meant for internal use only.

The thing is, I'm not sure what these functions are actually used for, and this help says "don't call them directly", but that sort of implies I could call them indirectly via some other function? Maybe it should just say "function is meant for internal use by the xyz function" or something like that, and then say "if you're looking for information about the PHP session variables, use the $_SESSION superglobal (I'm not clear from this doc, or maybe in general, what the relationship is between these functions and $_SESSION anyway).

daniels220’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

New patch reflecting how the functions are used.

jhodgdon’s picture

Status: Needs review » Needs work
+ * This function is registered with session_set_save_handler() to support
+ * database-backed sessions. It is called on every page load when PHP sets
+ * up the $_SESSION superglobal. It also sets up the $user global variable,
+ * including setting the user's authentication level.
+ *
+ * This function should not be called directly. Session data should instead be
+ * accessed via the $_SESSION superglobal.

It seems to me that the subjects covered here are (a) when/how the function is called (b) what it does and (c) whether you should use it and what you should do instead. I think that (a) and (c) belong together, and (b) is a separate topic. But the way it's arranged right now, (a) and (b) are in the first paragraph, and (c) is in the second. This doesn't make sense to me.

+ *   The user's session, or an empty string if no session exists (i.e. for
+ *   web crawlers and first-time visitors).

Should this really be i.e. (meaning "that is"), or e.g. (for example), or just left out? If you want to use either i.e. or e.g., the correct punctuation is:
i.e., for web ...

daniels220’s picture

Status: Needs work » Needs review
FileSize
1.43 KB

After finally getting Pro Drupal Development and reading a bit I understand these functions better. Specifically sess_read()'s role in setting up $user is quite important, so I moved that into the first sentence, and then the rest of the ordering worked out nicely.

jhodgdon’s picture

Status: Needs review » Needs work

The read function docblock needs to start with a one-sentence, one-line description. And there are a few trailing spaces in the patch that need to be removed. I recommend setting up your code editor so it automatically shows trailing spaces, if you can (emacs supports that, for instance).

Other than that, I think these function docs make sense... patch just needs a little cleanup/revision.

daniels220’s picture

Status: Needs work » Needs review
FileSize
1.46 KB

Try this.

Thanks for the suggestion, I can now see trailing spaces.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK, looks good! Thanks for all the iterations.

chx’s picture

Status: Reviewed & tested by the community » Needs work

I am sorry to be a pain in the behind but "should not be" is not strong enough by far. To supplement my grammar, I invoke http://tools.ietf.org/html/rfc2119 RFC 2119. Calling these functions can have unexpected consequences (especially sess_write) and they offer no value to the user. The text currently reads as "calling this function is not the right way, use SESSION instead" but the reality is "calling this function is a big no-no, it never works. I suspect you wanted to use SESSION". That's not instead. The two does not stand in any relation whatsoever.

This function must not be called directly. Calling this function might have totally unexpected consequences including corrupted session data. If you wanted to manipulate session data, then use the $_SESSION superglobal. These functions will not help with that or anything else.

jhodgdon’s picture

I think this makes sense... we can word it a bit more strongly.

daniels220’s picture

Status: Needs work » Needs review
FileSize
1.59 KB

Good point. Updated.

daniels220’s picture

Actually, here, slightly more complete I think.

jhodgdon’s picture

Status: Needs review » Needs work

I'm not sure about chx's wording though... How about:

This function should never be called directly -- it can have unexpected consequences and offers no value as a direct function call. Session data can be accessed via the $_SESSION superglobal.

jhodgdon’s picture

cross-posted.

I like that last version (patch in #18). chx - what do you think?

chx’s picture

Status: Needs work » Reviewed & tested by the community

I like it.

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Although there were some fuzzy mentions above of it not being applicable, as far as I see, _drupal_session_read() and _drupal_session_write() would do very well with these docs first.

jhodgdon’s picture

Ummm... I'm confused.

What does your marking of this to "patch (to be ported)"/D7 mean? Do you think we should be porting the D6 proposed patch above to D7 before we fix D6?

Gábor Hojtsy’s picture

Well, chx said "should not" is not strong enough, while D7 uses exactly that language, so...

jhodgdon’s picture

Ah, thanks. OK, need a re-patch of D7 then.

j0rd’s picture

sess_read / sess_write are recommended in a stack overflow response you get when googling for Drupal & Cookies. I would expect that a lot of people (including myself) have wasted time on this because neither function are documented properly in Drupal 6 documentation.

http://stackoverflow.com/questions/1317066/how-to-store-session-cookie-i...

Drupal should also add a proper page for handling cookies, so that it will get picked up in google and be the proper place to get this kind of information.

--
Drupal Commerce Development

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev

Can I just say that rather than complaining that something isn't done, the more productive thing to do would be to make a patch, which is what this issue is waiting for in order to be resolved?

Basically, we need a patch like #18 for Drupal 8 (for the functions mentioned in #22), at which point it can also be applied to Drupal 7, and then the patch in #18 can be applied to Drupal 6.

daniels220’s picture

Assigned: daniels220 » Unassigned

Sorry everyone, life happened. Sorry for leaving this "assigned" for so long.

Reidsy’s picture

This patch should close this issue.
It was based on 8.x but i'm guessing will also apply to 7.x.

jhodgdon’s picture

Status: Patch (to be ported) » Needs review

Triggering test bot

jhodgdon’s picture

#29: sess_read_write-825972-29.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I think this is good for d7/8, then should be marked back as RTBC for D6 (the patch for d6 should be reattached at that point from #18, at which point the bot will probably test it if the name doesn't have -D6 in it).

catch’s picture

#29: sess_read_write-825972-29.patch queued for re-testing.

catch’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: +Needs backport to D7

Committed to 8.x, this looks worth backporting to 7.x as well.

jhodgdon’s picture

Issue tags: +Needs backport to D6

Yeah, needs backporting to 6.x too. I'll hit retest to make sure the above patch applies to 7.x too. Then we need to mark patch/ to be ported and go back to #18 (which was RTBC but we need to reroll and run the test bot).

jhodgdon’s picture

#29: sess_read_write-825972-29.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks like a good clean-up.

Committed and pushed to 7.x. Thanks!

jhodgdon’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Still needs d6 backport.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.71 KB

D6 backport.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

That patch looks good for Drupal 6. Thanks!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed, pushed.

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D6, -Novice, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.