Make WordPress Core

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's profile SergeyBiryukov Owned by: joedolson's profile 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 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?

Attachments (6)

55078.diff (1.3 KB) - added by johnregan3 3 years ago.
Initial patch
55078-1.diff (1.5 KB) - added by johnregan3 3 years ago.
Updated version of previous patch.
55078-2.diff (1.5 KB) - added by johnregan3 3 years ago.
Updated patch using actual text as the message value.
55078-3.diff (2.5 KB) - added by johnregan3 3 years ago.
Updated to pass Unit Tests
55078.4.diff (916 bytes) - added by SergeyBiryukov 3 years ago.
55078.5.diff (625 bytes) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (35)

#1 @SergeyBiryukov
3 years ago

  • Type changed from defect (bug) to enhancement

#2 @SergeyBiryukov
3 years ago

  • Description modified (diff)

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


3 years ago

#4 @ryokuhi
3 years ago

  • Focuses accessibility added

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

#5 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 6.0

@johnregan3
3 years ago

Initial patch

#6 @johnregan3
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 @johnregan3
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,
)
Last edited 3 years ago by johnregan3 (previous) (diff)

@johnregan3
3 years ago

Updated version of previous patch.

#8 @johnregan3
3 years ago

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

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


3 years ago

#10 @sabernhardt
3 years ago

  • Keywords has-patch needs-testing added

#11 @SergeyBiryukov
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 @johnregan3
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.

@johnregan3
3 years ago

Updated patch using actual text as the message value.

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

@johnregan3
3 years ago

Updated to pass Unit Tests

#14 @johnregan3
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

#17 @joedolson
3 years ago

  • Owner set to joedolson
  • Status changed from new to accepted

#18 @joedolson
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: @joedolson
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: @peterwilsoncc
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 @peterwilsoncc
3 years ago

  • Keywords commit added

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.

#23 @peterwilsoncc
3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#25 @SergeyBiryukov
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: @peterwilsoncc
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 @SergeyBiryukov
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.

#28 @peterwilsoncc
3 years ago

  • Keywords commit added

55078.5.diff looks good for commit.

#29 @joedolson
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.