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
- Create a node. Let's say it's node 1.
- Go to
/node/1
- 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
Comment | File | Size | Author |
---|---|---|---|
#8 | core-quickedit-2551263-8.patch | 496 bytes | nod_ |
#2 | quickedit-destroy-2551263-1.patch | 551 bytes | Berdir |
Comments
Comment #2
BerdirHere 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.
Comment #3
legolasboI think we need solid steps to reproduce the issue in order to be able to do a thorough review.
Comment #4
Wim LeersBerdir'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.
Comment #5
Wim LeersI only debugged it to find the root cause. I did not write the solution.
Manually tested, this fixes the bug.
Comment #6
alexpottFails our eslint rules..
615:7 error Expected a conditional expression and instead saw an assignment no-cond-assign
Comment #7
alexpottThis passes the test.
Comment #8
nod_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
Comment #9
nod_Fixed the example, detachbehaviors expects a DOM object, not a jQuery object.
Comment #10
Wim LeersHah! Can't believe I didn't know that. Will do a new round of manual testing, unless Berdir beats me to it.
Comment #11
BerdirThat was a boring race ;)
Works!
Comment #12
Wim LeersHehe, sorry, I lost track of this one.
Comment #14
catchCommitted/pushed to 8.0.x, thanks!