Problem/Motivation

I'm not sure if it happens all the time or just when nodes exist multiple times on the same page, but we've been getting the following JS when e.g. a dialog that displays a node is removed from the page:

Uncaught DrupalBehaviorError: detach:unload ; quickedit: Cannot read property 'destroy' of undefined

Steps to reproduce

  1. Create a node. Let's say it's node 1.
  2. Go to /node/1
  3. Paste this in your browser's console:
    Drupal.detachBehaviors(jQuery('article[about="/node/1"]')[0])

    (This simulates that node living in a modal and the modal then being closed.)

Proposed resolution

WimLeers helped me to track this down

Narrowed it down to

    // Destroy all fields of this entity.
    this.get('fields').each(function (fieldModel) {
      fieldModel.destroy();
   });

It seems that as soon as the first fieldModel is destroyed, it then no longer is able to find the *next* one. i.e. the N+1th item became the Nth, but this tries to still use the N+1th item.
I'm betting that one of the Backbone or Underscore updates broke this.
Solution: http://stackoverflow.com/questions/10858935/cleanest-way-to-destroy-ever...

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 core-quickedit-2551263-8.patch496 bytesnod_
#2 quickedit-destroy-2551263-1.patch551 bytesberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new quickedit-destroy-2551263-1.patch551 bytes

Here is a patch, seems to work for me but I'm not sure if there is a way to reproduce this with just Drupal core.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Issue summary: View changes
Issue tags: +Needs steps to reproduce

I think we need solid steps to reproduce the issue in order to be able to do a thorough review.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Issue tags: -Needs steps to reproduce

Berdir's site uses a modal, and when the modal is closed, it invokes Drupal.detachBehaviors(), which causes this.

So, STR are simple. Added to IS.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

I only debugged it to find the root cause. I did not write the solution.

Manually tested, this fixes the bug.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Fails our eslint rules..
615:7 error Expected a conditional expression and instead saw an assignment no-cond-assign

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
-      this.get('fields').each(function (fieldModel) {
+      var fieldModel;
+      while ((fieldModel = this.get('fields').first()) != null) {
         fieldModel.destroy();
-      });
+      }

This passes the test.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
StatusFileSize
new core-quickedit-2551263-8.patch496 bytes

The eslint warning is because assignments in loops are ugly, since this is a backbone thing I looked around the documentation and there is a dedicated method to do this, reset.

The patch does call all the destroy methods for the collection and models and it's simple and readable :) http://backbonejs.org/#Collection-reset

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes

Fixed the example, detachbehaviors expects a DOM object, not a jQuery object.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs manual testing

Hah! Can't believe I didn't know that. Will do a new round of manual testing, unless Berdir beats me to it.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

That was a boring race ;)

Works!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Hehe, sorry, I lost track of this one.

  • catch committed 9f0bb4f on 8.0.x 
    Issue #2551263 by Berdir, nod_: Deleting entity/field models when markup...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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