Using session API to allow anonymous users to use Flag, the following error is thrown when flagging after cleaning cookies. I'm not sure if this is a problem with flag or with Session API.
PDOException: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'sid' at row 1: INSERT INTO {flag_content} (fid, content_type, content_id, uid, sid, timestamp) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => 2 [:db_insert_placeholder_1] => node [:db_insert_placeholder_2] => 600 [:db_insert_placeholder_3] => 0 [:db_insert_placeholder_4] => -1 [:db_insert_placeholder_5] => 1338975474 ) in flag_flag->_flag() (line 724 of sites/all/modules/flag/flag.inc).
Comment | File | Size | Author |
---|---|---|---|
#93 | 1619114.93.flag_.invalid-session-id.patch | 2.7 KB | joachim |
#90 | invalid-session-1619114-89.patch | 2.7 KB | a.ross |
#88 | invalid-session-1619114-88.patch | 1.12 KB | a.ross |
#79 | flag-fix_pdoexception_bug-1619114-79.patch | 913 bytes | shabana.navas |
#75 | flag-fix_pdoexception_bug-1619114-75.patch | 913 bytes | cspurk |
Comments
Comment #1
a.ross CreditAttribution: a.ross commentedI should note that it happens only when the flag is automatically created by Rules. It doesn't happen when manually flagged by the user. So this should probably be targeted at the rules integration.
Comment #2
a.ross CreditAttribution: a.ross commentedMore information:
We have a rule setup to flag content when viewed to be able to display a browsing history. Content is first unflagged and then flagged, to preserve the correct order. Anyway, when content is viewed once, the node is flagged for the first time. Then, when the user clears his cache and views the same content again, this error displays.
I've been able to reproduce this locally and it seems that manually unflagging the previously flagged content fixes the problem.
Comment #3
smartango CreditAttribution: smartango commented[copied from #1088672: Duplicate fid-node key error when flagging content ]
maybe i found something ..
adding
$create = TRUE;
at line 2041 of flag.module, make it works again
also I think there is no reason to be FALSE there
Comment #4
joachim CreditAttribution: joachim commentedShould a new session be requested at that point? I don't know enough about how Session API works to be sure.
Comment #5
smartango CreditAttribution: smartango commentedlooking at the code, in fact, session_api_get_sid with create setted to TRUE call
http://api.drupal.org/api/drupal/includes!session.inc/function/drupal_se...
that is safe, and should be called this way all time.
Actually this is the signature:
function session_api_get_sid($create = TRUE)
it would work:
Comment #6
smartango CreditAttribution: smartango commentedhowever I still get -1 from that function sometime (I can't say when), and this should never happen from definition of session_api_get_sid with params TRUE ..
Anyone experiencing this?
I do tested
if ($sids[$uid] == -1) $sids[$uid] =0;
as workaround and this "works"
Comment #7
a.ross CreditAttribution: a.ross commentedMy findings suggest that passing TRUE works. I only got the PDO error after cleaning cookies, but the error is now gone. However, I'm a bit confused about the code:
This function is only ever called with a single argument in flag.module, so why does it have two arguments? Also, it's just a wrapper around
flag_set_sid()
. Is this for legacy reasons? This is the function declaration offlag_set_sid()
.This function only gets called by
flag_get_sid()
, its wrapper, so the default value for $create is never even used.Anyway, I've attached a patch that does not touch all this, but it seems to me it could be much simpler.
Comment #8
joachim CreditAttribution: joachim commentedA get/set pair where one has a static and one is a wrapper is a common Drupal pattern. That's ok.
But I'm rather confused that the two have different parameters:
function flag_set_sid($uid = NULL, $create = TRUE) {
function flag_get_sid($uid = NULL, $create = FALSE) {
And I'm really unsure about changing the behaviour with session ids... what are the consequences?
Comment #9
a.ross CreditAttribution: a.ross commentedYeah, I've seen this pattern a number of times, but in this case it just seems redundant.
As far as consequences, I don't seem to be getting a new session upon every time I flag content, which is the only thing I can think of that could go wrong. However, when I clean cookies, it correctly recognizes a new session upon flagging.
Comment #10
smartango CreditAttribution: smartango commentedjoachim that make sense, so why do call flag_get_sid and not flag_set_sid? or why don't pass TRUE? (in _flag() method)
and having a sid equal -1 code should just leave and not try to set (so testing if sid is less than 0 would make error disappear, but things flag + session api do not work as expected: no flag)
Use of set variant is the better choice for me.
Comment #11
smartango CreditAttribution: smartango commentedjoachim I read the comment in #1088672-19: Duplicate fid-node key error when flagging content about the normal behaviour, this persuades me that it have to be called without parameters.
In http://drupal.org/node/319656 is not documented the availability of parameter.
also http://drupalcode.org/project/session_api.git/commit/81b1a9b2fb998984fb5... this commit says it is the port of the patch for #785292: Incompatible with Pressflow (create parameter), but I can't find when and if flag should use this flag setted to false, or do not care at all.
(finding this commit could explain more)
The -1 behavior is wanted, but, at least, it is not compatible with flag db table definition
Comment #12
vanul CreditAttribution: vanul commentedThe same bag, i don't know when it happens ....
PDOException: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'sid' at row 1: INSERT INTO {flag_content} (fid, content_type, content_id, uid, sid, timestamp) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => 3 [:db_insert_placeholder_1] => node [:db_insert_placeholder_2] => 17 [:db_insert_placeholder_3] => 0 [:db_insert_placeholder_4] => -1 [:db_insert_placeholder_5] => 1343879411 ) в функции flag_flag->_flag()
Comment #13
joachim CreditAttribution: joachim commentedThis was changed in commit 87783505a245e5bc5e2f02bc5c0a5ad337d6d9b2:
The issue is: #1258764: Flag starts a session on anonymous user loads which is bad for pressflow/varnish.
Given that d.org runs Pressflow, this isn't something we can change without understanding it.
Comment #14
joachim CreditAttribution: joachim commented> the following error is thrown when flagging after cleaning cookies. I'm not sure if this is a problem with flag or with Session API.
This must be because somewhere in the chain a session ID should be getting requested but isn't.
Could someone with this problem do a backtrace please?
Comment #15
a.ross CreditAttribution: a.ross commentedI've done a ddebug_backtrace() from the function _flag(). What's the generally accepted way to post backtraces on d.o?
Here's something at least, in the attachment is the HTML (without krumo js though):
26: flag_flag->_flag() (Array, 2 elements)
25: flag_flag->flag() (Array, 2 elements)
24: flag_node->flag() (Array, 2 elements)
23: flag_rules_action_flag() (Array, 2 elements)
22: call_user_func_array() (Array, 1 element)
21: FacesExtendable->__call() (Array, 2 elements)
20: RulesExtendable->__call() (Array, 2 elements)
19: RulesAction->executeCallback() (Array, 2 elements)
18: RulesAbstractPlugin->evaluate() (Array, 2 elements)
17: RulesActionContainer->evaluate() (Array, 2 elements)
16: Rule->evaluate() (Array, 2 elements)
15: RulesReactionRule->evaluate() (Array, 2 elements)
14: RulesActionContainer->evaluate() (Array, 2 elements)
13: RulesEventSet->executeByArgs() (Array, 2 elements)
12: rules_invoke_event() (Array, 2 elements)
11: rules_entity_view() (Array, 2 elements)
10: call_user_func_array() (Array, 1 element)
9: module_invoke_all() (Array, 2 elements)
8: node_build_content() (Array, 2 elements)
7: node_view() (Array, 2 elements)
6: node_view_multiple() (Array, 2 elements)
5: node_show() (Array, 2 elements)
4: node_page_view() (Array, 2 elements)
3: page_manager_node_view_page() (Array, 2 elements)
2: call_user_func_array() (Array, 1 element)
1: menu_execute_active_handler() (Array, 2 elements)
0: main() (Array, 2 elements)
Comment #16
smartango CreditAttribution: smartango commentedjoachim backtrace is 7 megs ... I had some difficulties understanding it ...
Anyway (thank you for finding that), summary says:
Currently, if flags are enabled for anonymous users there is a check on every page to see if the user has created a flag. This forces session_api to create and save session data for this anonymous user. It would be better for caching systems to only create that session data once the user actually flags something.
ok, fine. In flag.inc flag_get_sid is in 3 points, I just check this one:
flag_flag::access() method: return TRUE if user can flag the content, called as flag_get_sid($uid) (do not create). How do you check that user can or cannot flag a content, if you do not start session for anonymous user?
the code is:
why do not just pass TRUE there? or use flag_set_sid($uid) ?
if there's a need to check the user can flag or unflag a content before starting session, it should be done before (for both action), then if there is a chance (one of two), create the session and check what action is needed for that content.
Say add a method _actions_access($content_id,$account) return an array of access, if one is true, start sid and check if action needed is the one returned as true.
Is this acceptable? or just start session there?
Comment #17
joachim CreditAttribution: joachim commented> What's the generally accepted way to post backtraces on d.o?
That looks pretty good :)
But could you put the backtrace at the point where flag_get_sid() returns a -1 please?
Comment #18
a.ross CreditAttribution: a.ross commentedOk, what about this one:
Comment #19
joachim CreditAttribution: joachim commentedI've just tried to reproduce this and I can't.
Steps I took:
1. Open site in Safari
2. Flag some nodes without logging in
3. Cleared cookies.
4. Reloaded the page
5. Tried to flag a node.
Thanks for the backtrace.
Looks like flag_flag->access() calls flag_get_sid() without the parameter to get a new session ID if needed. But the thing is, I'm not sure flag_flag->access() can really know at this point whether it should or shouldn't ask for one. Or at least, I don't how how that should be determined.
Comment #20
a.ross CreditAttribution: a.ross commentedWell, the error I'm facing may be slightly different from the error smartango is having. I only get the error when flagging with Rules, as you can see in the former backtrace :)
Comment #21
smartango CreditAttribution: smartango commenteda.ross it is exactly the same : error on rule: access content -> flag content
Also, backtrace says when error happen, not when session should be started in order to not raise error.
flag_link should had already started the session for this .. why do you don't read my comment at #16 ?
using flag_set_sid in access method fix my problem, and I think yours too. If also has to be checked if anonymous user have a chance to set/unset a content before start the session to check what action has to be taken on that content, then it is needed a wider change (not so wide)
Comment #22
joachim CreditAttribution: joachim commented> flag_link should had already started the session for this
I'm not sure it should.
The change in the other issue I linked to is about not creating a session for an anon user *until* a flagging action takes place.
In other words, don't create one when outputting links.
Comment #23
smartango CreditAttribution: smartango commented> The change in the other issue I linked to is about not creating a session for an anon user *until* a flagging action takes place.
> In other words, don't create one when outputting links.
"using flag_set_sid in access method fix my problem .." is related to access method (called from flag_link)
the flag_link code comment says:
and in access method code there is:
if instead reverse the check: which action is permitted on content_id by user account (anonymous), if at least one, check state of content for this user.
To test the content_id flag status of an anonymous user YOU HAVE TO START SESSION there's no other way.
the original question (1095910):
Is there a way you could only start the session when something has actually happened that needs flag to record data?
has the answer: flag not only record data, also show data status to user, so session has to be start when there is data recorded, but you can not know if data is recorded if you do not start session ... or shortly: No.
The only option is to check if content shown is not flag-able and not unflag-able by anonymous, if not to both do not start session, otherwise .. yes.
or just state flag is incompatible with caching of pressflow.
.. I just want to make clean:
realistically access is called before any action taken. This change is tested and work. It could be a viable compromise
Comment #24
a.ross CreditAttribution: a.ross commented@smartango, I forgot for a moment that you'd mentioned Rules, but do you also get it after cleaning cookies? Because that's the only time I'm getting it (at least reliably).
Comment #25
joachim CreditAttribution: joachim commented> or just state flag is incompatible with caching of pressflow.
That's not going to fly -- both are in use on d.org.
> To test the content_id flag status of an anonymous user YOU HAVE TO START SESSION there's no other way.
Right, but when links are being output, if there's no session in existence, presumably the desired behaviour intended by #1258764: Flag starts a session on anonymous user loads which is bad for pressflow/varnish is to say 'If there's no session, there's no existing flagging data, therefore everything is unflagged'. And then only if the user tries to flag something should the session get created.
Could you post changes as patch files btw -- it does make it much easier to understand what changes you are suggesting.
Comment #26
smartango CreditAttribution: smartango commentedIt happen when I start private navigation, then open a page for which it apply a rule, rule is:
there is no other flagable content. When I visit that content type, I got PDOException. If I log in, logout, and visit a product (to which apply rule), I got PDOExcetion.
I tried to skip permission check on flag in the rule, but nothing changed.
Comment #27
a.ross CreditAttribution: a.ross commentedSmartango, could you try manually flagging and unflagging a product (with the standard flag buttons) and then see if the PDO exception still happens upon viewing the content? In my case, after manually flagging/unflagging, the exception is gone. Until I clear cookies again.
Comment #28
joachim CreditAttribution: joachim commentedIf it were possible to reproduce this bug without Rules involved, that would be a big help.
Comment #29
smartango CreditAttribution: smartango commentedok, the test is
- define another flag for other content type (article)
- show link on that content type
- visit an article
- visit product (error)
- flag article
- visit product: worked
So without rule involved it seems not possible to reproduce .. maybe something in action rule are different from flag via
http://rafaels.u84/flag/flag/flagarticle/13?destination=node/13&token=ec...
I also tested this, after restarting anonymous:
http://rafaels.u84/flag/unflag/flagarticle/13?destination=node/13&token=...
(copying link and changing flag to unflag). It worked and made rule flag work. (just to be sure, the rule first unflag then flag content, to update time)
Comment #30
smartango CreditAttribution: smartango commentedAfter some testing I can say that the problem is no flag_set_sid code:
(in pseudo diff syntax)
because it is good to have static for performance, but if sid is setteted for anonymous as -1 (there was a previous call to flag_get_sid()) it never get the chance to start the sessione even in real flag action, when it should.
just this change and thing should start to work.
session is not started in other case, such as visiting a content, thus it would be ok for the other bug
Comment #31
a.ross CreditAttribution: a.ross commentedMy rule originally had the unflag action also, but reproducing it on my test environment I omitted it.
Comment #32
joachim CreditAttribution: joachim commentedI think it would be better to not set a -1 into the static $sids array: so change this line:
Comment #33
smartango CreditAttribution: smartango commentedactually I cant immagine a scenario when this array is usefull at all, but maybe I have not a so wide vision ..
I prefer this:
if that check (this?) in return was there before, we never get notice of problem, and it would have been harder to find why flag are not setted for anonymous by a rule. Also (the rest of) code seems to works, and $sids array has a reason for uid 0: skip get sid call.
in fact returned sid is supposed to be 0 for no session or greater for session available, and test are in form of $sid != 0, $sid==0, and not $sid>0, $sid<=0 ... I suppose it could be a problem, but i do not know.
Comment #34
smartango CreditAttribution: smartango commentedthis one line change work, please test this patch in real world.
A test could be:
1. define a rule (with flag action)
2. visit a content that cause the action of the rule in 1. as anonymous user
with session_id enabled.
Comment #35
joachim CreditAttribution: joachim commentedLike I said, I'd rather we didn't store a -1 sid in the first place. It doesn't strike me as useful or meaningful in any way.
Also, complex expressions like that need a code comment in my book. Parsing that makes my head hurt :)
Comment #36
smartango CreditAttribution: smartango commentedwhich expression? this:
$sids not setted for that uid, or the function is called with create option TRUE AND $sids[$uid] was setted by a previous call of
session_api_get_sid();
(the only code that set it to -1).If not set on create we need:
not this way .. cleanup
But it look to me more complex than the patch, and not so efficient.
instead something like this:
ok, a lot of idea, the problem is how to take advance of static $sids and reduce function call, also having something that is stable and do not break other module. If $sids array exists it should (or could) be used elsewhere.
just in case, can I propose a greater change that make it efficent and clean? like:
session_api_get_sid is called at most 2 time, module_exists and session_api_available, at most 1 time, $sids array is maintained in a correct state (check syntax, I wrote down here in the html textarea, no check)
Comment #37
joachim CreditAttribution: joachim commentedYup, this:
> if (!isset($sids[$uid]) || ($create && $sids[$uid]==-1)) {
I'd like something that complex to come with a comment that explains what it's doing.
But like I said, I don't think this is the right way to go. Not that I'm sure my suggestion is right either .... We need to properly understand what flag_set_sid() is trying to do:
- it keeps static cache of sids set so far
- if the given $uid does not have a cached sid, it tries to get one.
-- if SessionAPI is installed and the user is anonymous, it gets one from session_api_get_sid()
-- otherwise, the session id is just 0.
So I would say this:
That's one extra line than the patch, yes, but that's really not a big deal.
EDIT: two extra lines, because that should have a comment on it too!
Comment #38
a.ross CreditAttribution: a.ross commentedHmmm: #1233650: Make sure Session API is available on the first page load
Comment #39
smartango CreditAttribution: smartango commented- it keeps static cache of sids set so far
- if the given $uid does not have a cached sid, it tries to get one.
-- if SessionAPI is installed and the user is anonymous, it gets one from session_api_get_sid()
-- otherwise, the session id is just 0.
there is something missing here:
- call flag_get_uid(0) [ which call flag_set_sid($uid,FALSE) ] returns 0 and set static $sids[0] to 0
- call flag_set_uid(0) [ which has $create = TRUE as default ] return 0 never start session [WRONG]
wrong because it cause the session to not start even if is intended by the call semantic: we should not care about what is setted in array, now it is called the set flavour and session should be started.
It is not the problem on how many line has a patch, but if it is the right code to do it. One can choose to reduce call number, or to be more elegant, (the better would be to achieve both), but at least it has to work in the right way.
If comment is missing this is the code:
but actually the last snippet in #36 has comment and it seems clean, given the scenario i described here (that leads to wrong behaviour).
I like the last snippet in #36 because it is efficient, as explained
Comment #40
joachim CreditAttribution: joachim commentedAh yes I think I see what you mean.
If session API returns a -1, we want to later on be able to come back here and this time create a sid.
Comment #41
sinasalek CreditAttribution: sinasalek commentedI confirm that #39 works perfectly fine for both authentication and non authenticated users
Comment #42
joachim CreditAttribution: joachim commentedNeeds making into a patch.
Comment #43
a.ross CreditAttribution: a.ross commentedI was thinking, if it's just about handling the -1 special case, why not just do this?
Comment #44
a.ross CreditAttribution: a.ross commentedThe patch for the above code.
Comment #45
a.ross CreditAttribution: a.ross commentedI also don't think it's a good idea to add another static. (and shouldn't statics use drupal_static?)
Comment #46
joachim CreditAttribution: joachim commentedThe problem with that is that -- as far as I understand -- for performance reasons we shouldn't be asking Session API for a sid unless we really need it.
Comment #47
a.ross CreditAttribution: a.ross commentedYes but aren't the results cached in the $sids static? So you would only have to set it once?
Otherwise I can create a patch of #39 and clarify the doc line a bit.
Comment #48
a.ross CreditAttribution: a.ross commentedPatch in the style of #39
Comment #49
a.ross CreditAttribution: a.ross commentedComment #50
smartango CreditAttribution: smartango commentedEhi boy, why do you reset your brain every time you post a commemt???
That is called with create true or false....
Please read what it's said before
Comment #51
a.ross CreditAttribution: a.ross commentedFirst of all, no need for offensive comments like that. Second, name the person you are addressing.
If you were referring to me, I created a patch of your code in #39, so I'm not sure what your point is.
Comment #52
smartango CreditAttribution: smartango commentedok I calm me down, and I apologize for my comment, but please do not close or commit patch before some test.
@a.ross I refer to #43 and #44, I was reading on cellphone, while a bit late for a visit.
But offensive comment maybe would hide the fear of wrong patch uploaded, and this path is not correct: to have a patch without at least two test cases (where it should set session, and where it shouldn't) is not a good practice.
Also with session_api dependencies, and without.
I stop blaming, I have to write a lot of tests in this moment, and I maybe I'll try to write some SimpleTest soon, I could spare time to write those for flag, with patch against git
Comment #53
sinasalek CreditAttribution: sinasalek commentedApplied patch #48 and problem resolved.
Also it did not cause any problem for authenticated users
tx @a.ross
Comment #54
joachim CreditAttribution: joachim commentedNeeds to go on 3.x first.
Also, I'm still not happy that I don't understand what a -1 means from session_api_get_sid(). Ideally I'd like that function to get some clearer documentation before this goes in. I don't want code I don't understand and can't maintain.
Comment #55
a.ross CreditAttribution: a.ross commented@Joachim, have you seen my comment in #38? Unfortunately, the session API issue queue seems a bit deserted.
Comment #56
joachim CreditAttribution: joachim commentedHeh, I even less want to be in charge of code that depends on code that's not maintained... :/
Comment #57
sinasalek CreditAttribution: sinasalek commentedWell, according to what i see in code flag already depends on session_api, lets not forget that 5,000 site are using session_api. So someone else will take over or fork it if it's really deserted. It's not that difficult to findout about it's status. All you gotta do is contacting the maintainer :)
Comment #58
joachim CreditAttribution: joachim commentedWell, someone who wants this patch to get in needs to contact the maintainer :D
Rules integration here is currently on minimal maintenance -- see #1736524: Rules maintainer / decouple Rules integration to a new project.
Comment #59
sinasalek CreditAttribution: sinasalek commentedIt don't think this patch is only related to rules.
It happens when an anonymous user flag a content
I also contacted session api maintainer, lets see how he's doing :)
Comment #60
squarecandy CreditAttribution: squarecandy commentedpatch #48 works for me.
Comment #61
PolPatch #48 is also working for me on 7.x-2.x.
Comment #62
joachim CreditAttribution: joachim commentedThat's great, and thanks for testing everyone, but I am still not happy with this code.
That means that the LAST time we came here, we returned an invalid ID. We're here now and finding an invalid ID in $sids because we previously came here and session_api_get_sid() gave us a -1, which we RETURNED, even though at the time it presumably was invalid too.
Which bit of Flag got the bad sid? What does it cause?
Has anyone contacted the maintainers of Session API to get clarification?
Comment #63
a.ross CreditAttribution: a.ross commentedI thought Smartango at least planned to write some tests, but he hasn't showed up in this issue anymore. As for Session API, I linked to this issue before: #1233650: Make sure Session API is available on the first page load, haven't looked into that myself since that, but it may very well be related. In that case the "fix" we're working on here could be a workaround.
Comment #64
joachim CreditAttribution: joachim commentedAt the very least I'd like the sort of logic in the patch at #44.
Clearly this issue affects a lot of people, so it would be good to get some sort of fix in. However, if SessionAPI has problems, and they affect us, it would be great to see that sorted. Don't suppose anyone wants to volunteer to co-maintain it? :)
Comment #65
smartango CreditAttribution: smartango commentedright a.ross, I like to be alerted, I planned tests for Sunday 9th.
EDIT 2012-12-09.
It is too much sunday here. I will reschedule this during first half of this week.
Test plan is:
• depends on session_api, rules, views (setUp() ok)
• create content type
• define flag
• define rule on content visited (definition in this issue)
• define the view (all flagged contents, order by flagged reverse)
• check no PDO exception
• check view for the content container in the view (".content .node-nid" as first child in view - check DOM tree)
Comment #66
joachim CreditAttribution: joachim commentedFor the content type, flag, rule, and view you can create a hidden test module that defines all these in standard hooks. Core does this for a few things.
Comment #67
cspurk CreditAttribution: cspurk commentedTest Case
Here is a test case for when I get the
PDOException
; I hope you can use that to reproduce the problem:hook_flag_access()
like this:test_flag
flag link.PDOException
.My explanation for the exception in this test case: when clicking the flag link,
MY_MODULE_flag_access()
(and also theaccess()
method of theflag_flag
) class) is called and will set the static$sids[$uid]
inflag_set_sid()
to-1
(viaget_user_count()
). Then the actual flagging action has to use this-1
SID which leads to the error.Solution Proposal
In reply to #62:
I haven’t contacted the Session API maintainer. Still I’d like to add my two cents on why I believe
-1
is returned fromsession_api_get_sid()
. Basicallysession_api_get_sid()
can return the following three values:<SOME_SESSION_ID>
$create == TRUE
FALSE
-1
$create == FALSE
(→ the Flag module’s problem)That’s the situation with which the Flag module has to cope: there are three cases/states to handle. If I’m not mistaken, then there is currently not a dedicated handling of the
-1
case. As noted above, the-1
case is the Flag module’s responsibility, though, so it needs to be handled internally.I would propose to entirely handle the
-1
case inflag_set_sid()
and to never show this state outside of this function. For the outside world, it could just look like theFALSE
case for now (which appears to be handled throughout the Flag module already). We still have to differentiate the two cases internally so that we can update the internally stored session ID upon request ($create == TRUE
). See the first attached patch to this comment which does exactly that and therefore eliminates thePDOException
. The second patch attached to this comment is required for the test case above to work as expected.Comment #68
cspurk CreditAttribution: cspurk commentedI think the problem is a core problem, not just related to the Rules integration component (see #67). I have also changed the status of this issue in order to notify the maintainers of my patches in #67.
Comment #70
smartango CreditAttribution: smartango commented@cspurk please try patch in #48 that already fix the problem, it does not?
Does you tested it separately?
I do not understand what is the test case, and on what it differs from the one I planned in #65, I mean depending on Rules module (and rule) or defining an hook, are these options really different?
The fact is that we can test with an available module (Rules) instead creating a new one (even if trivial), and test a real scenario, also the plan include a views and order on flag time.
Anyway we have still missing the test case, I wrote some code, first 3 step on plan (the simplest). Unfortunately, without a reproducible test case patches are not very appealing
Comment #71
joachim CreditAttribution: joachim commented> I would propose to entirely handle the -1 case in flag_set_sid() and to never show this state outside of this function. For the outside world, it could just look like the FALSE case for now (which appears to be handled throughout the Flag module already).
I like the sound of that.
Are both the patches in #67 part of the same fix? If so, they should be a single patch file rather than two.
Having a negative statement as a condition makes it harder to read -- could this be reversed?
Comment #72
smartango CreditAttribution: smartango commentedI think it is a kind of .. déjà vu. Does really have a -1 as return value cause the problem? from the code I worked on this is not really a problem.
What is called a "flag module problem", in #67 was discussed in this issue and it is due to the optimization, say: do not set session on testing flag.
Scenario:
- user visit page yoursite.domain/apage
- apage content test if a flag is setted by the anonymous user
- she/he is anonymous, it is impossible, then return -1 and all goes right
- now user click on bookmark (or the flag defined)
- being anonymous but with session_api module, the behavior should be to set flag any way
- code executed on flagging content is:
1. test flag status
2. set flag to that content
The patch attached in #48 has the effect of force creation of session id just in this scenario.
In other cases the session is not setted, and this has good implication on performance for anonymous user who do not really need the session id (such as the one just looking around the site).
This would lead to 2 test cases I think:
Case A. no session id setted for anonymous when flag is not requested to be setted
Case B. no exception when it is requested
Comment #73
smartango CreditAttribution: smartango commentedI write it better:
- code executed on flagging content is, during the single request (executiuon):
1. test flag status
2. set flag to that content
without the code in patch, module throws exception in the part of code that store in db session data, because session_id is negative.
Comment #74
smartango CreditAttribution: smartango commentedstressing it again and again ...
code in #26 #1619114-26: After cleaning cookies, flagging content as an anonymous user causes a PDO exception. does set the flag instead of asking for the availability, but I revise this solution because of the following discussion (comments that follow)
The code in http://drupal.org/files/flag-fix_get_user_count_session-1619114-67.patch (second patch):
has the mean of have a $sid not negative in a where clause in which code could not throw exception, because there is no violation of db constrain on checking a condition that could never be satisfied. So that second part is of no use. Right? or it has? maybe it make fail the test Case A in #1619114-72: After cleaning cookies, flagging content as an anonymous user causes a PDO exception.
Comment #75
cspurk CreditAttribution: cspurk commentedOkay, thanks all for your comments. I have further thought about the issue and done some more testing:
-1
SID to leak into the whole module (cf. #62).Well, that may be a question of taste. I find the relevant workflow of my test case easier to grasp than a workflow in which automatically firing rules and yet another (third-party) module are involved. The test case I have given in #67 also stems from a “real scenario” here.
Yes, I fully agree. My new patch doesn’t change the overall situation; it just doesn’t introduce any new SID case (
-1
) in the Flag module. There is still no session created – a0
SID means there is no session.Comment #76
smartango CreditAttribution: smartango commented@cspurk
Fine. Thanks. I got your point, and maybe is more simple to include a test module in the tree than define rule and views module in the test framework, thus reducing dependencies.
That said, I think your suggested test case (#67) should be split just in the last step (3.):
3a: re-load the same content and check no cookie are setted
3b: click on set flag and check no exception
and add some visible state in case of seted flag: thus causing the check-the-flag-code to be executed.
Comment #77
a.ross CreditAttribution: a.ross commentedRe-reading this issue again months later, I think the -1 return code should really be a constant (like SESSION_API_INVALID_SID) which has the -1 value, or -769042 for that matter. That constant should be referenced in documentation/API usage examples etc. That will make life a lot easier for the Flag module & friends.
Maybe it's a better idea to file a patch against session API first, which makes some headway in this direction.
Comment #78
vanul CreditAttribution: vanul commentedIt works. but why it's not at release?
Comment #79
shabana.navas CreditAttribution: shabana.navas commentedI can confirm the patch in #75 works just fine. I think it is a good resolution for now. I have re-rolled the patch for the 7.x-3.x-dev version. Hope Joachim can commit this.
Comment #80
PolWorking perfectly !
I also hope that this (#79) gets in asap !
Comment #81
joachim CreditAttribution: joachim commentedLooking good so far! Thanks for the patch, and for the review.
Let's do a bit more clean-up, to make this more maintainable in the future:
Can we take this opportunity to document the params and return of this function?
Does a -1 sid actually mean 'none was required'?
Since session_api_get_sid() appears to have incomplete docs (http://drupalcode.org/project/session_api.git/blob/refs/heads/7.x-1.x:/s...) I'd actually quite like it if we noted what the return values meant here.
Something like above the call to session_api_get_sid():
// This returns one of the following:
// - 0 if blah blah
// - A positive integer session ID if yadayada
// - -1 if something else.
(Obviously, SessionAPI needs fixing too... but I don't know how active that is.)
Given these are all documentation changes, I'm happy for a new patch to go straight back to RTBC :)
Comment #82
shabana.navas CreditAttribution: shabana.navas commentedI have contacted the sessions api maintainer for further clarification on the return values, so that we can put some accurate documentation in this patch. But, as of now, still no news from them. It doesn't look like the module is very active at the moment.
Once I do get the necessary information, I will go ahead and update the patch and post.
Comment #83
joachim CreditAttribution: joachim commentedI was thinking it's more a case of documenting what we're *assuming* the return values to mean, in the absence of decent docs in the module we're using.
(Need I say that having a key part of Flag's functionality depend on a module that is possibly unmaintained, and definitely badly documented, gives me the heebie-jeebies? I've filed #2043043: Missing documentation for session_api_get_sid().)
Comment #84
Pol#48 fixed the problem for me too.
Comment #85
joachim CreditAttribution: joachim commentedWould be nice to get this into 3.1, but as it's been around for ages, I'm not going to tag it as a blocker.
Comment #86
a.ross CreditAttribution: a.ross commentedI just sent a message to the Session API maintainer offering to become a co-maintainer. Awaiting his reply.
Comment #87
joachim CreditAttribution: joachim commentedI'm happy for this patch to get in with the changes in #81.
Comment #88
a.ross CreditAttribution: a.ross commentedI'll spare you Joachim, but the function could be much simpler than it currently is. If it will always return 0 for registered users, why not make a conditional for that at the top of the function? There's also no need to keep all UIDs in the
drupal_static()
, keeping a single item with the SID of the anonymous user suffices.Comment #89
a.ross CreditAttribution: a.ross commentedComment #90
a.ross CreditAttribution: a.ross commentedwow, that's a first... :)
Comment #91
PolYeah ! I can't wait to use the new version :) Thanks !
Comment #92
a.ross CreditAttribution: a.ross commentedI'm now co maintainer of the session API module, so I plan to go over it in the near future.
Comment #93
joachim CreditAttribution: joachim commented> but the function could be much simpler than it currently is. If it will always return 0 for registered users, why not make a conditional for that at the top of the function? There's also no need to keep all UIDs in the drupal_static(), keeping a single item with the SID of the anonymous user suffices.
I see what you mean. We could just put a trapdoor at the top:
I'll certainly take a patch for that in a new issue. It's probably not a big performance problem, as I expect that over one request we only come here for the current user (unless we're sending out emails with flag links or something?). But it certainly would help readability.
In the meantime, I'm committing this patch and finally laying this issue to rest. Patch is #90 with the @param docs tweaked to say '(optional)'.
git commit -m "Issue #1619114 by cspurk, a.ross, smartango, Shabana Blackborder: Fixed invalid session IDs causing exceptions when anonymous user no longer has a cookie." --author="git <git@2451710.no-reply.drupal.org>"
Thanks to everyone who's helped on this -- it's certainly been a long one!
And congrats a.ross on your new co-maintainership!
Comment #94
PolHell yeah ! Thanks !
Comment #95
a.ross CreditAttribution: a.ross commentedNice! I just noticed a small problem:
The default for the get function is FALSE, not TRUE.
Comment #96
joachim CreditAttribution: joachim commentedThanks for checking the patch and spotting that one! Fixed it.
While we're on those faffy params with their flip-flop, here's a potential follow-up:
There is only ONE call to flag_set_sid() in the whole module. $create is passed in. Hence on flag_set_sid(), there is no point in the param being optional. In fact, there is no point in either param being optional there.
Comment #97
a.ross CreditAttribution: a.ross commentedThat sounds a bit like what I was saying in #7. I think there may be a better solution though: use the function pair as intended throughout the module. There is one
flag_get_sid($uid, TRUE)
call in the module (includes/flag/flag_flag.inc:729), which can just be converted toflag_set_sid($uid)
.If you do that,
flag_get_sid()
won't need the second parameter at all.Comment #98
a.ross CreditAttribution: a.ross commentedBy the way, I filed this: #2072491: Simplify flag_set_sid()
We can take #96 and #97 over to that issue.
Comment #99
shabana.navas CreditAttribution: shabana.navas commentedHappy to see this finally committed :-)!
Comment #100
yana.me CreditAttribution: yana.me commentedFirst thank you for this great work and patches but I have to reopen this issue, as I come to the same problem with a PDO exception not from falgging content by anonymous user but with a newsletter confirmation.
We use the simplenews and flag module to get the last flagged newsletter in rules and to send it out when a new user has subscribed. Everything worked perfect until the moment I cleared the site wide Drupal cache in my admin menu. Now new subscribers fall in the PDO exception when they are confirming there subscription through the browser. As we configured rules to send emails to admin when a new subscription has been done, we can see that rules can't get the flagged content anymore. The log messages shows "Unable to get a data value. Error: Unknown data property flag_sid.".
I installed the latest dev version on a development server, applied all submitted patches but the error still exists. On the life server we are using the 7.x-3.1 version.
This is the first time I am writing on drupal.org, so please be a little bit patient with me.
I really appreciate all advises or help here, so thank you in advanced.
Comment #102
joachim CreditAttribution: joachim commentedPlease open a new issue for this -- though I have to say, this sounds like a case of doing things with flag that flag is not meant to do.
Comment #104
smussbach CreditAttribution: smussbach commentedBackport to Branch 2.x is discussed under https://www.drupal.org/node/2479559