backdrop-contrib / rules

Porting the Rules project to Backdrop CMS.
GNU General Public License v2.0
5 stars 10 forks source link

Events After saving / updating nodes of type happen before record is written to database #175

Open yorkshire-pudding opened 2 years ago

yorkshire-pudding commented 2 years ago

The UI for rules events is misleading.

image

There are several possible events that say "after" but the actions happen before. I confirmed this using Rules Throttle which I ported so I could check in a view whether the change had happened.

It is very misleading as there is also an event that says "before saving content". If all the "after" events actually take place before the save, what does the "before" event do?

I've come across this on nodes, but I think this could apply across the board on other entities. I couldn't find anything on our issue queue but checked on Drupal and found this: https://www.drupal.org/project/rules/issues/2877239

I then looked up our docs which both say:

Note that when this hook is invoked, the changes have not yet been written to the database, because a database transaction is still in progress. The transaction is not finalized until the save operation is entirely completed and node_save() goes out of scope. You should not rely on data in the database at this time as it is not updated yet. You should also note that any write/update database queries executed from this hook are also not committed immediately. Check node_save() and db_transaction() for more info.

https://docs.backdropcms.org/api/backdrop/core%21modules%21node%21node.api.php/function/hook_node_insert/1 https://docs.backdropcms.org/api/backdrop/core%21modules%21node%21node.api.php/function/hook_node_update/1

I have two questions:

  1. Is there any way in rules to do actions after the save event?
  2. Should we be looking to improve the UX here so that user expectations are managed?
argiepiano commented 2 years ago

@yorkshire-pudding thank you for posting. The Drupal issue you link contains the answer to your question: this is a problem with how Drupal handles database writing, and how it invokes hook_node_insert() etc. Rules uses that hook to trigger the "after" actions, but as pointed out in that post, sometimes the database writing transaction is not completed.

I have faced this issue in the past with Drupal (independent of Rules). In those occasions, I used a contrib module called Hook Post Action that assures an invocation after the db transaction is done. See https://www.drupal.org/project/hook_post_action

As a potential solution to this issue, we could write a new contrib Rules action event that is triggered by this contrib hook (we would need to port Hook Post Action).

In the meantime, it may be a good idea to clarify (somewhere) that the "after" actions may actually not happen after the db transaction is finished. Any thoughts on where to add such language?

yorkshire-pudding commented 2 years ago

@argiepiano - thank you for taking the time to consider this. I agree that clarification would be good in the short term; I believe adding here (Add/Edit Rule > Add Event) would be best:

image

It might make most sense to place the warning after this "Whenever the event occurs, rule evaluation is triggered."

The module has already been ported - https://github.com/backdrop-contrib/hook_post_action Would it be a contrib Rules 'event' rather than an 'action' as presumably you'd want site builders to be able to use the event for any suitable action?

argiepiano commented 2 years ago

@yorkshire-pudding good to know. Yes, I meant event, not action.

argiepiano commented 2 years ago

@yorkshire-pudding as for the placement of the clarification: putting it after "Whenever the event occurs..." doesn't seem possible. This is displayed in the form created by rules_ui_add_event(), which is a form not tied to the actual events.

The API for hook_rules_event_info() includes a 'description' key within the key variables (in this case, provided by rules_events_node_variables()). It may be possible to add a description there (in the return of rules_events_node_variables())

I don't have time to try this right now, but feel free to experiment adding a description to that array in that function.

yorkshire-pudding commented 2 years ago

@argiepiano - would it be possible to expand the description attribute in line 506? https://github.com/backdrop-contrib/rules/blob/7026f0577a9044324fa20dd40c22f155a7d1a70d/ui/ui.forms.inc#L506

Yes, it would be there all the time, but I think having a valid qualifier to the statement "Whenever the event occurs, rule evaluation is triggered." is beneficial. As it is, it is misleading as these triggers don't happen after the database write.

bugfolder commented 6 months ago

Adding a "more" expansion toggle to the description that explains the subtleties of database writing timing would be a good solution to this. Yet another reason why it would be great to get https://github.com/backdrop/backdrop-issues/issues/5090 into core.

argiepiano commented 3 months ago

One more comment in case others find this issue, just to avoid the idea that there is something "wrong" with this Rules event.

Yes, the database transaction may not be completed when this event is triggered, BUT the node object that is passed to the rule contains a nid and all the fields. For most purposes, this is enough to take actions: for example, modifying another entity, or even sending an email. Now, if you try to load the same node that was saved from the database, you may run into issues at this point, the same issues you would face by using hook_node_insert() or hook_node_update().

Generally, in Rules as well as in implementations of those hooks, you want to avoid using node_load() to load the same node that is being passed to the rule or the hook.

The title of this issue is also a bit misleading. By the point this rule is triggered, the node has already been saved but the database transaction may be incomplete. The node has already been assigned a nid by this point.