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

Feature/auto initialize modules #158

Merged
merged 12 commits into from
Nov 18, 2022
Merged

Conversation

darylldoyle
Copy link
Collaborator

Currently there is very little PHP scaffolding in the repo, especially in the MU-Plugin. This means that to set up, engineers usually end up copy/pasting the scaffold from other projects. This PR proposes a starting point for a less obstructive PHP scaffold, where PHP classes are automatically registered based on conditionals.

Description of the Change

This PR does the following:

  • Adds a new abstract Module class that defines can_register() and register() methods.
  • Utilizes the haydenpierce/class-finder package to find all classes within a namespace (currently TenUpPlugin)
  • Of all the found classes, find those that extend the Module class.
  • Of those classes, find those where the can_register() method returns true.
  • Calls the register() method on all those remaining.

There is some documentation available within the docs directory, see here.

This is related to #4

How to test the Change

Clone the repo into a WordPress install and try registering a class using the new method.

Changelog Entry

Added - Auto-registration of PHP classes in the MU-plugin

Credits

Props @darylldoyle

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@dsawardekar
Copy link

@darylldoyle I think this is a step in the right direction. It would make modules easily manageable like single file mu-plugins. One thing that stood out to me was the composer.json requirement. On most projects it is not an issue to set this as a deploy step.

I would suggest adding an admin_notice if composer is not found in case it hasn't been setup or failed to load etc.

docs/registering-classes.md Outdated Show resolved Hide resolved
mu-plugins/10up-plugin/includes/core.php Outdated Show resolved Hide resolved
@darylldoyle
Copy link
Collaborator Author

@darylldoyle I think this is a step in the right direction. It would make modules easily manageable like single file mu-plugins. One thing that stood out to me was the composer.json requirement. On most projects it is not an issue to set this as a deploy step.

I would suggest adding an admin_notice if composer is not found in case it hasn't been setup or failed to load etc.

Thanks for the notes @dsawardekar! I've updated the PR to fix those and also to add the loader to the theme as suggested by @gthayer.

If either of you have another chance for a review, that'd be much appreciated!

Copy link

@dsawardekar dsawardekar left a comment

Choose a reason for hiding this comment

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

@darylldoyle This looks good to me. It will add much needed standardization to the MU plugin. And it is also pretty lightweight and does not require a big ramp up.

@joesnellpdx joesnellpdx merged commit b952066 into trunk Nov 18, 2022
@joesnellpdx joesnellpdx deleted the feature/auto-initialize-modules branch November 18, 2022 18:08
@tlovett1
Copy link
Member

tlovett1 commented Mar 2, 2023

Hey all, I'm realizing 0 documentation was added for this. Can someone please do that ASAP? @darylldoyle @dsawardekar

@darylldoyle
Copy link
Collaborator Author

@tlovett1 there is some documentation available in https://github.com/10up/wp-scaffold/blob/trunk/docs/registering-classes.md.

I can also write up a blog post for the internal blog if that'd help?

Is there anything additional you'd like to see added to the documentation?

@dsawardekar dsawardekar restored the feature/auto-initialize-modules branch March 31, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants