Opened 3 years ago
Closed 3 years ago
#55078 closed enhancement (fixed)
Remove hardcoded check for edit-tags.php from lib/ajax-response.js
Reported by: | SergeyBiryukov | Owned by: | joedolson |
---|---|---|---|
Milestone: | 6.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Administration | Keywords: | has-patch needs-testing commit |
Focuses: | accessibility, javascript | Cc: |
Description (last modified by )
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?
Attachments (6)
Change History (35)
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
3 years ago
#6
@
3 years ago
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
#7
@
3 years ago
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, )
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
3 years ago
#11
@
3 years ago
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.
#12
@
3 years ago
@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.
This ticket was mentioned in PR #2408 on WordPress/wordpress-develop by johnregan3.
3 years ago
#13
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
#14
@
3 years ago
@SergeyBiryukov Updated patch to pass PHPUnit tests. Also issued a PR for further discussion as needed.
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
3 years ago
This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.
3 years ago
#18
@
3 years ago
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.
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
3 years ago
#20
follow-up:
↓ 21
@
3 years ago
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?
#21
in reply to:
↑ 20
;
follow-up:
↓ 22
@
3 years ago
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.
#22
in reply to:
↑ 21
@
3 years ago
- Keywords commit added
Replying to peterwilsoncc:
Given
data
needs to remain, I'd be inclined to leave it out ofsupplemental
for now.
🤦 Nevermind, I see....
I've refreshed the linked pull request against trunk and changed the variable name.
peterwilsoncc commented on PR #2408:
3 years ago
#24
#25
@
3 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
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.
#26
follow-up:
↓ 27
@
3 years ago
- Keywords commit removed
@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.
#27
in reply to:
↑ 26
@
3 years ago
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.
As this ticket is the follow-up to two tickets related to accessibility, we're adding the accessibility focus to keep track of it.