Closed jmichaelward closed 3 years ago
The first thing I noticed is we've got a merge commit bringing 0.2.1
into this branch. I'd love to see a rebase here instead - I find it helps me track the changes more easily (and it makes for a cleaner history, which makes the uptight part of me happy 😀).
I'm still reviewing with regards to content.
I'd love to see a rebase here instead - I find it helps me track the changes more easily (and it makes for a cleaner history, which makes the uptight part of me happy 😀).
@salcode Done, thanks.
This is a refactor of @tommcfarlin's original pull request here: https://github.com/WebDevStudios/oops-wp/pull/18
There's quite a bit of new changes happening here. This is a general list:
MetaBox
class from Content to its own subdirectory, MetaBox.ContentType
implementation with a new interface:MetaBoxInterface
, which extends theRegisterable
andRenderable
interfaces.__construct
method from the abstract class. Additional details below.meets_requirements
method tocheck_requirements
, and adds additional requirements checks.add_meta_box
function call into its own private method to ensure it cannot be changed or broken by extending classes.set_dynamic_properties
method which gets called right before the requirements check. Additional details below.save
functionality, which is something I'd like to talk about on the BEE call.Additional Details
First, I'd like to thank Tom for taking a first pass on the MetaBox abstraction, and I would also like to apologize that it has taken this long to get a refactor in place. I'm hopeful that these changes will allow us to move forward and get this introduced into the next release of OOPS-WP.
As with the other structures in OOPS-WP, the goal for this MetaBox class is to make registering custom metaboxes within WordPress as seamless as possible. Since these classes generally represent some kind of model, we want the concrete instances to define all of their own properties, so that engineers who need to work on updating a particular part of the model can look at a focused section of the codebase.
Class Constructor
In the original PR, MetaBox properties were being passed in to the constructor. In most other cases, I think this is the correct approach, but I chose to remove the constructor in this refactor for a couple reasons:
Because of the way we handle registration of these ContentTypes, we facilitate the engineering experience by throwing exceptions when properties on the class are forgotten or incorrectly defined. By eliminating the separation between those definitions, I think we can provide better clarity to help engineers more easily register metaboxes when they need them.
Updated Exception Checking
Three properties are required to define a MetaBox: an $id, a $title, and a $screen. The optional values of a MetaBox, although optional, still must abide by a contract within WordPress, so we've introduced exception handling for properties that are incorrect.
Contract Updates
Tom correctly determined in his original PR that all MetaBoxes must be rendered, but the original PR did not require extending objects to implement a
render
method. That is corrected in this PR - theMetaBoxInterface
extendsRenderable
, so PHP will throw an error if concrete MetaBoxes do not create this method.Additionally, I replaced the original extension of
ContentType
with the newMetaBoxInterface
implementation for a few reasons:ContentType
objects have a different method signature that requires the passing in of array arguments.MetaBox
registration in WordPress is very explicit about the arguments it accepts. Using theget_default_args
andget_args
format of the otherContentType
elements is contrary to the goals of OOPS-WP, which is to make it easier to register these elements. Associative array usage dictates the need to remember the specific keys and structures engineers must try to implement, and since metaboxes are extremely explicit, I felt it best to follow that same structure here.MetaBox
can have the responsibility of saving content, it is inherently itself not aContentType
. In hindsight, it's possible that aTaxonomy
is not aContentType
, either, even if it might share the same interface as aPostType
. We might need some continued discussions around this. Importantly, though, aMetaBox
could be registered simply to display instructions or other kinds of information to a user in the Dashboard, and as such, it is a distinctly different structure.ContentType
, then was forced to implement a method it didn't need. This breaks the Interface Segregation Principle of SOLID - itself a code smell that suggested a new interface was needed. Hopefully we've addressed that here by creating the newMetaBoxInterface
.Additional Work Is Needed
I think this is a proper first version of a Metabox class, but acknowledge that there is more work to consider. In Tom's PR, he indicated the need for some (most?) metaboxes to save data, and as such, the requirement to register hooks to do so during given events.
For other structures, we typically call
register_hooks
from within aService
- for instance, aContentRegistrar
might createPostType
andTaxonomy
objects during theinit
action, so thatService
would create a callback method to perform those operations and callregister
each object.How a given
MetaBox
saves data, however, is somewhat different. I'm not sure it's the responsibility of aMetaBoxService
to know the details about saving a givenMetaBox
's data, but I also don't know that it's the responsibility of theMetaBox
either - its job is to register itself, to know how to render on the screen, and to know which arguments it needs for a callback. I think we should have some conversations about what this looks like. Should aMetaBox
have an event handler of some type? Should it register hooks during itsregister
procedure if it has hooks to register? Not allMetaBox
objects will need to save or care about events in WordPress, so that may need to be at the engineer's discretion on a case-by-case basis. There's nothing that prevents an engineer from implementingHookable
on a concrete object.Let's discuss this further during the BEE scrum this week so we can get more ideas and feedback. That said, I'd appreciate some eyes on this PR so we can determine whether it meets the general short-term need of registering metaboxes within WordPress.