Remove hardcoded check for edit-tags.php from lib/ajax-response.js

[SergeyBiryukov]

Background: #42937, #54955.

As a result of [52170] and [52672], _enqueues/lib/ajax-response.js has a hardcoded check for edit-tags.php​, which seems unnecessarily specific. As noted in comment:6:ticket:54955, this is less than ideal and was added as a temporary measure.

We should find a better way to pass the success message in an Ajax response, that could also be reused in other places. Maybe passing it as part of the supplemental data would work?

[ryokuhi]

As this ticket is the follow-up to two tickets related to accessibility, we're adding the accessibility focus to keep track of it.

[johnregan3]

Initial patch

[johnregan3]

Added initial patch. I'm not satisfied with the naming ("data_is_success_message"), but for now I wanted to use something clear while evaluating this approach to a solution for this ticket. Definitely open to input. @SergeyBiryukov

[johnregan3]

Also, I think I'd like to replace the compact() on line 1120 with something more readable.

'supplemental' => array(
     'parents'                 => $parents,
     'noparents'               => $noparents,
     'data_is_success_message' => true,
)

[johnregan3]

Updated version of previous patch.

[johnregan3]

This new patch (55078-1) fixes an error in the original patch.

[SergeyBiryukov]

Thanks for the patch! This looks like a good start to me, but I think it would be a bit cleaner if the message parameter of supplemental contained the message itself, instead of it being passed in the data parameter.

[johnregan3]

@SergeyBiryukov Gladly. I went that route bc that's how the JS was written originally. I'll issue a patch within the next business day or two.

[johnregan3]

Updated patch using actual text as the message value.

Removes hardcoded check for edit-tags-php body class from ajax-response.js. Instead allows for a "notice" value in the "supplemental" array.

Renamed the JS var successmsg to noticemsg to clarify its purpose as admin notice text.

Note in ajax-actions.php around ln 1118, $message is used twice. When the data value is removed from the array, tests fail in tests/phpunit/tests/ajax/AddTag.php. TBH, I can't track down exactly why.

Trac ticket: https://core.trac.wordpress.org/ticket/55078

[johnregan3]

Updated to pass Unit Tests

[johnregan3]

@SergeyBiryukov Updated patch to pass PHPUnit tests. Also issued a PR for further discussion as needed.

[joedolson]

I'm not seeing why removing data from that array is triggering a tests failure, either.

Looks like it can be set to empty, since it's overridden in the response. It certainly feels like it should be omittable if it's going to be empty, but since it's a default argument, the handler will put it back in the array in the add method. So in aggregate, I don't think there's any benefit to omitting the argument.

Might need a comment to explain the duplication and/or omission, however, unless we identify why the test is failing.

[joedolson]

For the record, I think that the tests failure is in some way related to changing the shape of the AJAX response, and that leaving the parameter in the response is the best solution here, but I can't find a definite answer to that in the unit tests.

@costdev @hellofromTonya @boniu91 Any opinions on the tests issue?

[peterwilsoncc]

Replying to joedolson:

For the record, I think that the tests failure is in some way related to changing the shape of the AJAX response, and that leaving the parameter in the response is the best solution here, but I can't find a definite answer to that in the unit tests.

I agree, removing it is an unnecessary backward compatibility break that may affect more than WordPress Core. Changing the shape by removing the data ought to happen on a dedicated ticket.

Given data needs to remain, I'd be inclined to leave it out of supplemental for now.

If the variable is to be renamed, I suggest noticeMessage for both observing coding standards and clarity.

[peterwilsoncc]

Replying to peterwilsoncc:

Given data needs to remain, I'd be inclined to leave it out of supplemental for now.

🤦 Nevermind, I see....

I've refreshed the linked pull request against trunk and changed the variable name.

[peterwilsoncc]

In 53123:

Administration: Remove term page check from ajax-response.js.

Replace hard coded check for the term creation page in _enqueues/lib/ajax-response.js with a check for a notification to display in the AJAX response data.

Follow up to [52170], [52672].

Props SergeyBiryukov, ryokuhi, johnregan3, sabernhardt, joedolson.
Fixes #55078.
See #54955.

Merged in https://core.trac.wordpress.org/changeset/53123

[SergeyBiryukov]

Thanks everyone!

I don't see a reason for 'data' => $message to remain, it was only added five months ago in [52170] and never documented anywhere, so should not affect backward compatibility. The duplication might also cause confusion later.

I think the test just needs to be updated to check the supplemental->notice property instead, see 55078.4.diff​. With this change, the tests pass as expected.

[peterwilsoncc]

@SergeyBiryukov As I mentioned above, any hard deprecations need to bed discussed on a dedicated ticket. I am specifically blocking doing so on random tickets in my role on the 6.0 release squad.

I am leaving this ticket to add, not replace, the test for supplemental data.

[SergeyBiryukov]

Replying to peterwilsoncc:

As I mentioned above, any hard deprecations need to bed discussed on a dedicated ticket.

Ah, good point, I missed the note about a dedicated ticket. Thanks!

Added a new test assertion in 55078.5.diff​. Created #55560 for further discussion.

[peterwilsoncc]

55078.5.diff​ looks good for commit.

[joedolson]

In 53159:

Administration: Add unit test for term supplementary notice.

Adds a new test assertion to handle the supplemental data array passed in ajax-response.js.

Follow up to [53123].

Props SergeyBiryukov, peterwilsoncc.
Fixes #55078.