Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a feature class to represent standard feature behaviors #75

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

dlh01
Copy link
Member

@dlh01 dlh01 commented Feb 8, 2024

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

  • I updated the README.md file for any new/updated features.
  • I updated the CHANGELOG.md file for any new/updated features.

Changelog entries

Added

  • Alley\WP\Alleyvate\Feature class implementing the Alley\WP\Types\Feature interface.

Changed

  • The minimum PHP version is now 8.1.
  • Feature classes now implement the Alley\WP\Types\Feature interface instead of Alley\WP\Alleyvate\Feature.

Removed

  • site_health: Removed as a dedicated feature and now implemented directly in the plugin.
  • Alley\WP\Alleyvate\Feature interface.

@dlh01 dlh01 requested a review from a team as a code owner February 8, 2024 08:08
@dlh01 dlh01 force-pushed the feature/feature-class branch from d404f4f to 0ef1d62 Compare February 8, 2024 12:44
@dlh01 dlh01 force-pushed the feature/feature-class branch from 0ef1d62 to 43e3c57 Compare February 8, 2024 18:56
Copy link
Contributor

@mogmarsh mogmarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this looks much cleaner!

Copy link
Contributor

@renatonascalves renatonascalves left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗡️

@dlh01 dlh01 merged commit 8a82cd4 into main Feb 21, 2024
7 checks passed
@dlh01 dlh01 deleted the feature/feature-class branch February 21, 2024 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants