I think a note need to be added to explain that is better to use $_SESSION[] than sess_write and sess_read
#139587: sess_read() logs users out
http://newriversdigital.com/content/content/session-data-drupal-do-not-u...
In D7 apparently this is a private function now
Comment | File | Size | Author |
---|---|---|---|
#39 | sess_read_write-825972-d6-39.patch | 1.71 KB | Albert Volkman |
#29 | sess_read_write-825972-29.patch | 2.63 KB | Reidsy |
#18 | sess_read_write_new5_825972_D6.patch | 1.7 KB | daniels220 |
#17 | sess_read_write_new4_825972_D6.patch | 1.59 KB | daniels220 |
#13 | sess_read_write_new3_825972_D6.patch | 1.46 KB | daniels220 |
Comments
Comment #1
gagarine CreditAttribution: gagarine commentedThe pages are http://api.drupal.org/api/function/sess_write and http://api.drupal.org/api/function/sess_read
Comment #2
jhodgdonThis 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.
Comment #3
jhodgdonProbably a good project for a novice doc contributor
Comment #4
chx CreditAttribution: chx commentedsess_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.
Comment #5
gagarine CreditAttribution: gagarine commentedExactly 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.
Comment #6
Jaypan CreditAttribution: Jaypan commentedThat'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.
Comment #7
daniels220 CreditAttribution: daniels220 commentedHere's a patch introducing complete docblocks for the functions. Is my choice of wording at the top good?
Comment #8
jhodgdonI'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).
Comment #9
daniels220 CreditAttribution: daniels220 commentedNew patch reflecting how the functions are used.
Comment #10
jhodgdonIt 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.
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 ...
Comment #11
daniels220 CreditAttribution: daniels220 commentedAfter 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.
Comment #12
jhodgdonThe 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.
Comment #13
daniels220 CreditAttribution: daniels220 commentedTry this.
Thanks for the suggestion, I can now see trailing spaces.
Comment #14
jhodgdonOK, looks good! Thanks for all the iterations.
Comment #15
chx CreditAttribution: chx commentedI 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.
Comment #16
jhodgdonI think this makes sense... we can word it a bit more strongly.
Comment #17
daniels220 CreditAttribution: daniels220 commentedGood point. Updated.
Comment #18
daniels220 CreditAttribution: daniels220 commentedActually, here, slightly more complete I think.
Comment #19
jhodgdonI'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.
Comment #20
jhodgdoncross-posted.
I like that last version (patch in #18). chx - what do you think?
Comment #21
chx CreditAttribution: chx commentedI like it.
Comment #22
Gábor HojtsyAlthough 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.
Comment #23
jhodgdonUmmm... 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?
Comment #24
Gábor HojtsyWell, chx said "should not" is not strong enough, while D7 uses exactly that language, so...
Comment #25
jhodgdonAh, thanks. OK, need a re-patch of D7 then.
Comment #26
j0rd CreditAttribution: j0rd commentedsess_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
Comment #27
jhodgdonCan 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.
Comment #28
daniels220 CreditAttribution: daniels220 commentedSorry everyone, life happened. Sorry for leaving this "assigned" for so long.
Comment #29
Reidsy CreditAttribution: Reidsy commentedThis patch should close this issue.
It was based on 8.x but i'm guessing will also apply to 7.x.
Comment #30
jhodgdonTriggering test bot
Comment #31
jhodgdon#29: sess_read_write-825972-29.patch queued for re-testing.
Comment #32
jhodgdonI 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).
Comment #33
catch#29: sess_read_write-825972-29.patch queued for re-testing.
Comment #34
catchCommitted to 8.x, this looks worth backporting to 7.x as well.
Comment #35
jhodgdonYeah, 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).
Comment #36
jhodgdon#29: sess_read_write-825972-29.patch queued for re-testing.
Comment #37
webchickThis looks like a good clean-up.
Committed and pushed to 7.x. Thanks!
Comment #38
jhodgdonStill needs d6 backport.
Comment #39
Albert Volkman CreditAttribution: Albert Volkman commentedD6 backport.
Comment #40
jhodgdonThat patch looks good for Drupal 6. Thanks!
Comment #41
Gábor HojtsyThanks, committed, pushed.