alleyinteractive / wp-alleyvate

Defaults for WordPress sites by Alley.
GNU General Public License v2.0
16 stars 2 forks source link

Use a feature class to represent standard feature behaviors #75

Closed dlh01 closed 4 months ago

dlh01 commented 5 months ago

Summary

From what I've observed, there are (at least) three pieces of behavior that are standard to every Alleyvate feature:

  1. It loads on after_setup_theme.
  2. It can be preempted with the alleyvate_load_* filters.
  3. Its status is represented in the plugin's Site Health panel.

These behaviors are currently spread across the plugin — in wp-alleyvate.php, load.php, and the site health feature class, mainly.

This PR proposes a refactor to create a single class that represents these standard behaviors.

First, it replaces the Alleyvate Feature interface with the one from WP Type Extensions, which is otherwise identical.

Second, it creates an Alleyvate Feature class. This is a decorator class, taking the passed feature and adding to it the standard behaviors mentioned above.

The primary benefit of this change, in my opinion, is greater cohesiveness. As mentioned, given how the plugin is currently structured, there isn't an obvious relationship among these different bits of logic, but actually they all play a role in making Alleyvate features work the way they do. The single class helps to make that relationship clear.

A secondary benefit is to continue to promote interoperability across Alley code via standard interfaces — WP Curate and Create WordPress Plugin now use the Feature definition from Type Extensions as well.

(Aside: One of the stated design goals of WP Type Extensions is to encourage the use of object composition and decorators. This proposal is also meant as an application of that pattern with a project-specific, built-for-purpose class capable of building on more-generic classes of its type.)

A third benefit is that the Site Health integration can no longer be turned off. It seemed a little miscast to me as an Alleyvate feature because it's diagnostic information, not functionality "essential to delivering a project meeting Alley's standard of quality." The approach taken here bakes the site health integration into the plugin rather than treating it like other changes to WordPress core functionality provided by Alleyvate that a developer might need to evaluate for their site.

As for the downsides of this approach, one of them is that each feature now adds a separate action to after_setup_theme, rather than having a single action encompassing all features, which is a bit less performant and a bit noisier in the logs.

Also, the removal of the site health feature would likely necessitate that the next version of the plugin be a major version (edit: so will the change in minimum PHP version to 8.0, to accommodate WP Type Extensions). In practice, I think only someone who was disabling the site health integration should expect to see any change in plugin behavior resulting from this PR, so there would be little risk in updating. At the same time, there would be little user-facing reward, perhaps making the update only an inconvenience for teams.

Notes for reviewers

None.

Other Information

Changelog entries

Added

Changed

Removed