Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When a node is being inserting, it shouldn't be flagged, but flag_nodeapi is calling flag('unflag') if $node->flag[flag_name] = false.
Comment | File | Size | Author |
---|---|---|---|
#11 | flag-insert-1896146-10-access.patch | 6.38 KB | hefox |
#9 | flag-insert-1896146-9.patch | 5.68 KB | hefox |
#7 | flag-insert-1896146-7.patch | 407 bytes | hefox |
#7 | flag-insert-1896146-7-notempty.patch | 1.59 KB | hefox |
flag_nodeapi_insert.patch | 628 bytes | hefox | |
Comments
Comment #1
socketwench CreditAttribution: socketwench commentedDoesn't this need a break statement before the update case?
Comment #2
socketwench CreditAttribution: socketwench commentedComment #3
hefox CreditAttribution: hefox commentedNo, 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.
Comment #4
joachim CreditAttribution: joachim commentedI 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.
Comment #5
hefox CreditAttribution: hefox commentedI 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 :)
Comment #6
joachim CreditAttribution: joachim commentedComment #7
hefox CreditAttribution: hefox commentededit: ignore patch 2, was having a stupid moment.
Comment #9
hefox CreditAttribution: hefox commentedI 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
Comment #11
hefox CreditAttribution: hefox commentedUpdating to skip permission check in flag_field_attach_save like flag_node_save did
Comment #12
joachim CreditAttribution: joachim commentedThis has suddenly become a big patch which changes lots of things for reasons I don't understand...
What's happened to these?
Adding this parameter is covered by another issue.
Sorry, I don't understand at all why this has been moved out of the flag class.
Comment #13
hefox CreditAttribution: hefox commentedTo 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.
Comment #14
joachim CreditAttribution: joachim commented> 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).
Comment #15
joachim CreditAttribution: joachim commented> 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():
-- but I would be quite happy to fold the special handling for nodes into flag_field_attach_form().
Comment #16
hefox CreditAttribution: hefox commentedI 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.