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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Status: Active » Needs review
FileSize
1.08 KB
andypost’s picture

Version: 8.1.x-dev » 8.4.x-dev
Issue tags: -Needs manual testingcript +Needs manual testing
GrandmaGlassesRopeMan’s picture

Status: Needs review » Needs work
+++ b/core/misc/ajax.es6.js
@@ -92,9 +92,7 @@
+          delete Drupal.ajax.instances[instance.instanceIndex];

Since this is an array the delete operator won't work here. You probably want to use something like Array.splice(), assign that result to a variable and then null it out, but I'm sure there's some nuanced memory implications there.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.09 KB

re #4

Since this is an array the delete operator won't work here.

I was confused because manual testing was showing me it was working

But looking at delete operator - JavaScript | MDN
I saw

Deleting array elements

When you delete an array element, the array length is not affected. This holds even if you delete the last element of the array.

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.

droplet’s picture

It's tricky, very tricky.

const ajaxInstances = ['instanceIndex-0', 'instanceIndex-1', 'instanceIndex-2', 'instanceIndex-3'];

instanceIndex = 1;
ajaxInstances.splice(instanceIndex , 1');
//  ['instanceIndex-0', 'instanceIndex-2', 'instanceIndex-3'];

instanceIndex = 2;
ajaxInstances.splice(instanceIndex , 1'); 
// ['instanceIndex-0', 'instanceIndex-2'];

See the problem? the length is changed, the element shifts one position. It removed the wrong target.

Needs new test enhancement :)

tedbow’s picture

Status: Needs review » Closed (works as designed)

@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.

droplet’s picture

@droplet sorry

that'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