[...] 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.

See the discussion at #1619114: After cleaning cookies, flagging content as an anonymous user causes a PDO exception. for background.

Files: 
CommentFileSizeAuthor
#4 2072491-simplify_flag_set_sid-4.patch3.33 KBa.ross
PASSED: [[SimpleTest]]: [MySQL] 266 pass(es).
[ View ]
#3 2072491-simplify_flag_set_sid-3.patch3.33 KBa.ross
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2072491-simplify_flag_set_sid-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 2072491-simplify_flag_set_sid-1.patch1.74 KBa.ross
PASSED: [[SimpleTest]]: [MySQL] 266 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.74 KB
PASSED: [[SimpleTest]]: [MySQL] 266 pass(es).
[ View ]

From #1619114: After cleaning cookies, flagging content as an anonymous user causes a PDO exception.

Posted by joachim on August 23, 2013 at 10:53am

While we're on those faffy params with their flip-flop, here's a potential follow-up:

return flag_set_sid($uid, $create);
// ...
function flag_set_sid($uid = NULL, $create = TRUE) {

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.

Posted by a.ross on August 23, 2013 at 12:08pm

That 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 to flag_set_sid($uid).

If you do that, flag_get_sid() won't need the second parameter at all.

StatusFileSize
new3.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2072491-simplify_flag_set_sid-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

StatusFileSize
new3.33 KB
PASSED: [[SimpleTest]]: [MySQL] 266 pass(es).
[ View ]

Needs re-roll.

  1. +++ b/flag.module
    @@ -2284,29 +2284,29 @@ function flag_check_token($token, $entity_id) {
    function flag_set_sid($uid = NULL, $create = TRUE) {

    This is where I think we can lose the $create parameter.

  2. +++ b/flag.module
    @@ -2318,17 +2318,14 @@ function flag_set_sid($uid = NULL, $create = TRUE) {
    -function flag_get_sid($uid = NULL, $create = FALSE) {
    -  return flag_set_sid($uid, $create);
    +function flag_get_sid($uid = NULL) {
    +  return flag_set_sid($uid, FALSE);

    Can we definitely lose the $create here? Do we always call it with FALSE?

This is where I think we can lose the $create parameter.

Can we definitely lose the $create here? Do we always call it with FALSE?

Note that the solution I propose is slightly different from yours. I propose that the module should use flag_get_sid($uid) and flag_set_sid($uid) everywhere, depending upon what's necessary (setting or merely getting). So you do away with using the second parameter, because you're now using the get/set pair properly :)
In that case you MUST have the second parameter for the flag_set_sid() function, as that is called by flag_get_sid() with $create = FALSE. But you can do away with the second parameter for flag_get_sid() altogether.

Note these changes:

+++ b/flag.module
@@ -2508,6 +2505,6 @@ function flag_properties_get_flagging_users($entity, array $options, $name, $ent
-  $sid =  flag_get_sid($entity->uid, FALSE);
+  $sid =  flag_get_sid($entity->uid);

Second parameter passed value is the same as the default value (FALSE). Redundant.

+++ b/includes/flag/flag_flag.inc
@@ -726,7 +726,7 @@ class flag_flag {
-      $sid = flag_get_sid($uid, TRUE);
+      $sid = flag_set_sid($uid);

Second parameter TRUE implies setting, so this can be converted to flag_set_sid().