The problem

Case:

  1. User “ZZZ” had 11 or more nodes (articles) which he is an author of.
  2. User “ZZZ” deleted with “Cancel the selected user account(s)” - “Delete the account and make its content belong to the Anonymous user.”
  3. Expected result: Nodes previously belong to “ZZZ” should belong to “Anonymous”.
    Real result: Nodes previously belong to “ZZZ” disappeared.

Drupal log entry:
Warning: Parameter 5 to _node_mass_update_batch_process() expected to be a reference, value given in _batch_process() (line 252 of /.../core/includes/batch.inc).

Proposed resolution

While node count is less than 11 nodes are deleted in a loop; otherwise nodes deleted using batch.

For the time being temporary patch:

--- a/<html>core/modules/node/node.admin.inc (<b>Today 5:05:56 PM</b>)</html>
+++ b/<html><b>Current File</b></html>
@@ -33,7 +33,7 @@
 function node_mass_update(array $nodes, array $updates, $langcode = NULL, $load = FALSE, $revisions = FALSE) {
   // We use batch processing to prevent timeout when updating a large number
   // of nodes.
-  if (count($nodes) > 10) {
+  if (FALSE && count($nodes) > 10) {
     $batch = array(
       'operations' => array(
         array('_node_mass_update_batch_process', array($nodes, $updates, $langcode, $load, $revisions))
CommentFileSizeAuthor
#20 2688535-19.patch4.5 KBalexpott
#9 interdiff.txt1.29 KBamateescu
#9 2688535-9.patch4.5 KBamateescu
#5 2688535-5.patch4.51 KBamateescu
#5 2688535-5-test-only.patch2.86 KBamateescu
#4 2688535-4.patch1.65 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

olegiv created an issue. See original summary.

olegiv’s picture

olegiv CreditAttribution: olegiv commented
Priority: Normal » Major
Issue summary: View changes
Issue tags: -user, -deletion +user delete, +delete
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Priority: Major » Critical
Issue tags: -user delete, -batch, -node, -delete

Unexpected data loss is critical priority.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Version: 8.0.5 » 8.0.x-dev
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2688535-4.patch1.65 KB

The problem is that _node_mass_update_batch_process() is missing a $langcode parameter, which should've been added in #1498674: Refactor node properties to multilingual.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Issue tags: -Needs tests +D8MI
FileSize
2688535-5-test-only.patch2.86 KB
2688535-5.patch4.51 KB

And with a test.

The last submitted patch, 5: 2688535-5-test-only.patch, failed testing.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Wow, that was fast!

+++ b/core/modules/node/node.admin.inc
@@ -111,7 +114,7 @@ function _node_mass_update_helper(NodeInterface $node, array $updates, $langcode
-function _node_mass_update_batch_process(array $nodes, array $updates, $load, $revisions, array &$context) {
+function _node_mass_update_batch_process(array $nodes, array $updates, $langcode = NULL, $load, $revisions, array &$context) {

I think the coding standard is that parameters with default values must be at the end of the argument list.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Oh, I see, it's how the calling function works. It may not be possible to "fix" #7.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2688535-9.patch4.5 KB
interdiff.txt1.29 KB

Well, the new parameter doesn't have to be optional, a default is provided by node_mass_update() anyway.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Great work @amateescu, and thanks @olegiv for reporting this.

Can we add the parameter at the end as an optional one instead? Otherwise this is an internal API break, which makes it potentially disruptive for a patch release. And we definitely want to fix this in 8.0.x if we can, because data loss. The function is prefixed with an underscore so it's not a public API break, but this is the kind of "internal" API helper that contrib/custom code is prone to actually rely on.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs change record

To clarify #10, I think there would be a change within node_mass_update() as well just reordering the parameters in this line:

        array('_node_mass_update_batch_process', array($nodes, $updates, $langcode, $load, $revisions))
      ),

That way the $langocde could be added optionally. The downside is the parameters would be in a different order than the caller.

If that doesn't make sense, this is the kind of issue we would actually break APIs for if needed, but best to avoid it if possible.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Hmmm OTOH maybe $langcode shouldn't be optional, if failing to provide it results in unexpected deletions of other content. So in that case a hard API break with a change record would actually be preferable, to ensure that any sites incorrectly using that helper do update it to receive the langcode correctly. Thoughts?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Hmm I'd probably go for #9 in 8.1.x and higher. Then the point about actively breaking things in 8.0.x to prevent data loss is a good one, so would go for it there too.

If you get a 500 error from some contrib/custom code that's not updated calling this function, that's not really worse than it unexpectedly deleting content. It should never be called during regular site operation - is pretty rare, so it's only going to break the mass update, not the regular functioning of the site.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Issue tags: -Needs change record

I did consider making the new parameter optional and reorder the calling code, but I also think a hard break is preferable here for the reasons mentioned in #13.

Here's the change record: https://www.drupal.org/node/2692565

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Triaged D8 critical

@alexpott, @effulgentsia, @Cottser, @catch and I all agreed that this issue is critical because of the risk of data loss.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

The fix looks good and we have test coverage. And given the that we are already calling this with the param and all the functions are underscored I think adding the param makes sense.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2688535-9.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.0.x-dev » 8.1.x-dev

The last submitted patch, 9: 2688535-9.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Reviewed & tested by the community
FileSize
2688535-19.patch4.5 KB

Rerolled.

  • catch committed c9252d0 on 8.2.x 
    Issue #2688535 by amateescu, alexpott, xjm, cilefen, olegiv: User...

  • catch committed ff7f234 on 8.1.x 
    Issue #2688535 by amateescu, alexpott, xjm, cilefen, olegiv: User...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

Published the CR :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.