Make WordPress Core

Opened 7 years ago

Last modified 4 years ago

#38923 new enhancement

Introduce `WP_Action_Handler` to clean up admin action mess

Reported by: flixos90's profile flixos90 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch 2nd-opinion reporter-feedback
Focuses: Cc:

Description

We all know that several parts of the admin code are messy in a way. After we had to take several looks at these files during today's multisite chat, I thought we need to do something about it.

My proposal for now is to introduce a new base class WP_Action_Handler. This abstract class would lay a foundation to handle any kind of actions on any admin screen. Currently there's a (sometimes huge) switch clause at the beginning of almost every page. By removing these clauses entirely and handling the logic in a dedicated class for this instead, we get the following benefits:

  • easier readable code
  • less huge files
  • possibility to add unit tests for each of the switch cases
  • possibility to add unit tests the entire handling of an action
  • easy addition of any custom actions in plugins, beyond just bulk actions (which have become easier to implement thanks to 4.7)

I have been experimenting with such a class over the past few hours and will upload a first patch below. Some notes:

  • The class usage is almost similar to all the list table classes: There's a base class, and then there's (or rather there will be) one class (which extends the base class) for each screen that could benefit from that.
  • Many of the actions have similarities: They mostly use check_admin_referer() at the beginning, followed by some capability and other permission checks to verify the user is allowed to perform the action. Therefore the base class should be able to handle nonces on its own. The capability checks should be separated from the actual action logic for better organization and easier testing.
  • Many of the actions redirect back to the admin page, with a GET parameter denoting a notification message to show to the user. WP_Action_Handler should also be capable of managing such messages to make that easier as well. This will furthermore allow to remove a bunch of code from the admin screen files where they "bootstrap" their supported messages.

More notes will follow on the initial patch.

Attachments (1)

38923.diff (57.3 KB) - added by flixos90 7 years ago.

Download all attachments as: .zip

Change History (4)

@flixos90
7 years ago

#1 @flixos90
7 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

38923.diff shows what I have in mind. The core of this patch are the new files wp-admin/includes/class-wp-action-handler.php and wp-admin/includes/action-handler.php.

To test the behavior of the new class and to build it in a way that makes sense, I also created implementations for two screens, user-new.php and post.php. They both have their own action handler classes, so the code two handle their actions has been moved into these classes and the screen files are much shorter now as a result.
Functionality-wise there have not been any changes in the patch, everything should work exactly as before.

While the action handler class was made specifically for use-cases in the way that user-new.php works, it also brings significant improvements in terms of testing and extendability to post.php although that file works completely different (no messages, basically the entire file is an action clause). The functionality for the edit action was left in the main file since this is not really just an action, but rather displays the entire regular admin screen.

I could probably describe a lot more here, but looking at the code will likely give the best overview anyway. If there are any questions about the approach, please ask. Also feel free to tear it apart - this patch should of course be the opener for a discussion.

Regardless of how we will scaffold such a class, I think this will definitely be a valuable improvement.

#2 @dd32
7 years ago

I'm not sure abstracting this away is a good use of developer time.

It's working as-is right now, and going forward there'll more reliance upon using the REST API for most of these kind of actions.
We're probably at the point where a no-js fallback action isn't likely to be needed in a good proportion of cases.

#3 @valentinbora
4 years ago

  • Keywords reporter-feedback added

@flixos90 it's been a while since this got any attention, so could you please share updated thoughts over this?

Note: See TracTickets for help on using tickets.