Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June. 27 /**
28 * Return a Session API ID corresponding to the current session.
29 * The session id is a created using the same method as Drupal core
30 * as defined in includes/session.inc.
31 * @param boolean create set this to FALSE if you don't want to create a session
32 * cookie if it doesn't already exist.
33 */
34 function session_api_get_sid($create = TRUE) {This is not properly documented. We are having problems at Flag (#1619114: After cleaning cookies, flagging content as an anonymous user causes a PDO exception.) because we don't properly understand the return values of this function.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 2043043-6.patch | 1.12 KB | a.ross |
| #5 | 2043043-5.patch | 1.13 KB | a.ross |
| #3 | 2043043-3.patch | 5.3 KB | a.ross |
Comments
Comment #1
a.ross commentedThis needs an
@return. I advocate using a constant for the-1return value as well.Comment #2
joachim commentedThat would be a change in functionality, rather than just docs, so best left to another issue. I'm happy with it being an actual -1, as long as it's documented.
Comment #3
a.ross commentedHm, why would it be a change in functionality? It will still return -1 (and be perfectly backward-compatible as such), it will just be made possible to properly document that return value using a constant.
I've been working on a patch today, here's what I came up with.
Comment #4
joachim commentedThis patch seems to be changing code as well as docs.
Personally, I dislike defining constants for things like that, but it's up to the maintainer. It still doesn't belong in this issue though.
Comment #5
a.ross commentedYeah, what I wanted to do was to add some inline documentation as well. But as the code was pretty unorganized I felt it needed to be restructured. Doing this did help me understand the code better though and it let me to believe that this module needs a revamp.
Anyway, let's keep this issue focused on the function documentation. Here's a new patch where I actually finished (ahem) that bit.
Comment #6
a.ross commentedComment #7
a.ross commentedFixing bad line break
Comment #8
joachim commented> it let me to believe that this module needs a revamp.
I've not looked at it enough to either agree or disagree with you on that :)
But speaking from my own experience as a module maintainer who is often pushed for time, and often is looking at patches for modules whose code he's not looked at in ages, the smaller and simpler the patches, the easier it is for a maintainer to commit them. If someone posts a docs-only patch, I'll commit it pretty much straight away after a quick check that it makes sense. A bigger patch that changes code will often get put off because I don't feel I have enough time to properly get my head round it.
So I don't want to discourage you, but I think it's better to do revamping in a follow-up issue :)
Comment #9
a.ross commentedI completely agree with you on that, I just kinda forgot about the scope of the issue while working on that first patch.
Comment #10
a.ross commentedThe tests confirm that the function behaves in the way that this patch documents. Committed with a slight change.