Problem/Motivation
Follow-up to #2561619: Drupal Ajax objects and settings grows endlessly
We found in #2897938: Prevent accessing null elements when operating on ajax instances using the off_canvas renderer that Drupal.ajax.instances can start to have a bunch of elements set to null.
This happens when you open a dialog that has an ajax link or form submit and then remove the dialog, probably with other case besides dialogs.
Originally we were handling these null but in #2844261: Allow dialog links to specify a renderer in addition to a dialog type we changed this code and the check for NULL was removed.
It is really unintuitive that these nulls would be left in the array.
Currently in #2897938: Prevent accessing null elements when operating on ajax instances using the off_canvas renderer we are adding back the check for null. But it would be much simpler if Drupal.ajax.instances just contained actual instances.
Proposed resolution
Remove these elements instead of setting to null.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#6 | 2898242-6.patch | 1.09 KB | tedbow |
#2 | 2898242-2.patch | 1.08 KB | tedbow |
Comments
Comment #2
tedbowComment #3
andypostComment #4
GrandmaGlassesRopeManSince this is an array the
delete
operator won't work here. You probably want to use something likeArray.splice()
, assign that result to a variable and thennull
it out, but I'm sure there's some nuanced memory implications there.Comment #6
tedbowre #4
I was confused because manual testing was showing me it was working
But looking at delete operator - JavaScript | MDN
I saw
so I was seeing the element in the array being removed but I didn't check the array length. Tricky
I tested this new patch manually. It removes the element and lowers the array length.
Comment #7
droplet CreditAttribution: droplet commentedIt's tricky, very tricky.
See the problem? the length is changed, the element shifts one position. It removed the wrong target.
Needs new test enhancement :)
Comment #8
tedbow@droplet sorry I really missed that each element in Drupal.ajax.instances is keeping track of it's own index in the array.
So I would say that since it was easy fix in #2897938: Prevent accessing null elements when operating on ajax instances using the off_canvas renderer to work around this that we don't change anything here.
We could really create some tricky bugs in contrib if we change anything here.
Comment #9
droplet CreditAttribution: droplet commentedthat's okay :) It's really a tricky design.
Just my quick thought..
we able to add an extra "expire" key to the ajax instance. Therefore, we need not the instanceIndex key to remove it.
Anyway, in a project like drupal, sometimes I think (always) checking the element exists before other actions doesn't a bad thing..
#2897938: Prevent accessing null elements when operating on ajax instances using the off_canvas renderer