When a node is being inserting, it shouldn't be flagged, but flag_nodeapi is calling flag('unflag') if $node->flag[flag_name] = false.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

socketwench’s picture

+++ b/flag.moduleundefined
@@ -448,8 +448,12 @@ function flag_form_alter(&$form, &$form_state, $form_id) {
+        $node->flag = array_filter($node->flag);
+      }
+    case 'update':

Doesn't this need a break statement before the update case?

socketwench’s picture

Status: Needs review » Needs work
hefox’s picture

Status: Needs work » Needs review

No, on insert want to flag anything that has been marked to flag and on update want to flag anything marked to flag, unflag anything marked to unflag so all for insert remove anything that isn't "flag me" (e.g. the unflag for 'update') and fall through to the foreach in update to flag away.

joachim’s picture

I don't really see the advantage of this, as it just complicates the logic around insert/update.

At any rate, would need looking at on 7x3x first.

hefox’s picture

I haven't checked 7x, but in 6.x I tracked this down via flag('unflag' causing a few database queries (I don't think it did a delete, forgot exactly), and an array_filter is a lot cheaper than a database query. The faster a node inserts, the better for everyone :)

joachim’s picture

Version: 6.x-2.x-dev » 7.x-3.x-dev
Status: Needs review » Needs work
hefox’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
407 bytes

edit: ignore patch 2, was having a stupid moment.

Status: Needs review » Needs work

The last submitted patch, flag-insert-1896146-7-notempty.patch, failed testing.

hefox’s picture

Status: Needs work » Needs review
FileSize
5.68 KB

I noticed that $remember in flag_node_save means the node entity is only remembered once, for the first flag, meaning the second flag needs to load the node, so I went to patch that... when I found out that the first call to fetch_entity was from flag_field_attach_save, so as far as I can tell flagging is trying to happen twice (flag_field_attach_save and flag_node_save).

So updating patch to move the array_filter to flag_field_attach_insert, remove flag_node_save, and add a shred cache flag_remember_entity so all flags can just use one cache (no reason to use more memory than necessary)

untested

Status: Needs review » Needs work

The last submitted patch, flag-insert-1896146-9.patch, failed testing.

hefox’s picture

Status: Needs work » Needs review
FileSize
6.38 KB

Updating to skip permission check in flag_field_attach_save like flag_node_save did

joachim’s picture

Status: Needs review » Needs work

This has suddenly become a big patch which changes lots of things for reasons I don't understand...

+++ b/flag.module
@@ -972,46 +985,6 @@ function flag_link($type, $entity = NULL, $teaser = FALSE) {
 /**
- * Implements hook_node_insert().
- */
-function flag_node_insert($node) {
-  flag_node_save($node);
-}
-
-/**
- * Implements hook_node_update().
- */
-function flag_node_update($node) {
-  flag_node_save($node);
-}

What's happened to these?

+++ b/flag.module
@@ -1274,12 +1247,12 @@ function flag_field_attach_delete_bundle($entity_type, $bundle, $instances) {
+function flag($action, $flag_name, $entity_id, $account = NULL, $skip_permission_check = FALSE) {

Adding this parameter is covered by another issue.

+++ b/includes/flag/flag_flag.inc
@@ -366,7 +367,7 @@ class flag_flag {
-    $this->fetch_entity($entity_id, $object);
+    flag_remember_entity($this->entity_type, $entity_id, $object);

Sorry, I don't understand at all why this has been moved out of the flag class.

hefox’s picture

Status: Needs work » Needs review

To restate (as I do address most of these issues in the comments above)

1) right now things are (trying to) be flagged twice -- once in flag_field_attach_save and once in flag_node_save, so seems like flag_node_save is deprecated (the trying is due to access check that flag_field_attach_save was done but flag_node_save wasn't)

2) To fully deprecate flag_node_save
it's either add the param or change flag_field_attach_save to use $flag->flag like flag_node_save

3) Part of removing flag_node_save for flag_field_attach_save. There is a bug in flag_node_save; look how $remember is used -- it only remembers for one $flag. Even fixing that, it means each flag instance has to have their own private static cache, which memory management wise seemed quite unideal (even though during how it's being used above the memory would likely be freed). didn't really see a reason to replicate bad logic so fixed it up a bit.

joachim’s picture

Status: Needs review » Needs work

> 1) right now things are (trying to) be flagged twice -- once in flag_field_attach_save and once in flag_node_save, so seems like flag_node_save is deprecated (the trying is due to access check that flag_field_attach_save was done but flag_node_save wasn't)

That sounds reasonable enough, but it's out of scope of this issue. If the param change is needed for that, then it's a dependent issue (which as I mentioned already exists).

joachim’s picture

> right now things are (trying to) be flagged twice -- once in flag_field_attach_save and once in flag_node_save

Actually, I am not sure that is the case, due to this bypass in flag_field_attach_form():

  // Nodes have their own special form handling: see flag_form_alter().
  if ($entity_type == 'node') {
    return;
  }

-- but I would be quite happy to fold the special handling for nodes into flag_field_attach_form().

hefox’s picture

I found out about flag_field_attach_insert via a debug_backtrace in fetch_content when I was looking into the issue with $remember in flag_node_save

Note that all the tests passed with #11, which does test flagging on insert, indicating that it does work w/o flag_node_save (I tested most of it manually also).

So currently, flag_form_alter is adding the flag element to nodes whereas flag_field_attach_form is adding for non-nodes, but since flag_field_attach_save doesn't check that entity type != node, both flag_field_Attach_save and flag_node_save are flagging on insert/update.

So my patch cleans that later part out, but leaves the form alters as is as they don't relate to my original goal -- which, for me, is actually making the node_save better/more performant in 6.x.

  • Without addressing the flag_field_attach_save, the node would still try to flag(unflag, on insert.
  • Without addressing the remember content stuff, would need to replicate buggy/unoptominal code in flag_field_attach_save.
  • The adding argument to flag would have needed to change flag_field_attach_save to do flag load and $flag->, which I find uglier to read so adding the argument seemed cleaner. #1689530: flag() should support $skip_permission_check is the issue (and it doesn't break API's, as adding an additional argument that has a default value -- I can't see how that'd break anything; it's the same order as ->flag()).