Opened 8 years ago
Last modified 7 years ago
#40527 new enhancement
Decouple WP_Customize_Manager
Reported by: | utkarshpatel | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 4.8 |
Component: | Customize | Keywords: | needs-patch |
Focuses: | Cc: |
Description
When you create multiple instances of WP_Customize_Manager
it adds/removes multiple hooks in its constructor which should be executed single time.
So let's say if I want to create two new changeset posts it I will do.
<?php $data = array( ... // Some changeset data. ); $manager1 = new WP_Customize_Manager(); $manager1->save_changeset_post( array( 'data' => $data ) ); $manager2 = new WP_Customize_Manager(); $manager2->save_changeset_post( array( 'data' => $data ) );
This will cause wp_ajax_customize_save
to add twice with $manager1
and $manager2
it should add only once.
wp_ajax_customize_save
is a just example.
See Constructor: https://github.com/WordPress/WordPress/blob/5f771393a318d333503d5e13525dfcd543819302/wp-includes/class-wp-customize-manager.php#L229-L369.
So we should decouple WP_Customize_Manager
class and maybe extract changeset methods in separate class WP_Customize_Changeset
.
Attachments (2)
Change History (12)
#1
@
8 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
#2
@
8 years ago
Part of the scope here is to invalidate any cached data when clean_post_cache
is called. See https://github.com/WP-API/wp-api-customize-endpoints/pull/5#pullrequestreview-40207192
#3
@
7 years ago
This can also include factoring out the logic for trashing a changeset, as @dlh has prototyped in https://github.com/WP-API/wp-api-customize-endpoints/pull/5
#5
@
7 years ago
40527.diff is a first attempt at a WP_Customize_Changeset
class.
Some notes about the patch:
- The scope so far has been to abstract as much changeset code as possible that operates independently of a
WP_Customize_Manager
instance. I chose this scope in part based on the comment here: https://github.com/WP-API/wp-api-customize-endpoints/pull/5#discussion_r118804446.
- There is, of course, room to add more functionality from
WP_Customize_Manager
-- for instance, more of the logic fromsave_changeset_post()
. But because doing so also might increase the coupling between the two classes, I thought it best to check in first.
- One exception to the above is the
publish()
method, which accepts a$wp_customize
instance.
- There are two named constructors for instantiating an instance based on a UUID or a post ID. Core needs to be able to look up changeset data by both UUID and post ID, and in neither case is there guaranteed to be a corresponding post in the database. I found it easier to handle the logic for what the changeset instance should look like under these different circumstances in the separate constructors rather than a single
__construct()
, but I'm open to other ideas.
- The patch doesn't try to swap a
WP_Customize_Changeset
into every location in core where it could be used, so it might not handle all the requirements it would need to yet. Attempting to integrate the class into the in-progress REST API endpoints would also probably uncover additional needs or other functionality that would be helpful.
- No unit tests yet.
This ticket was mentioned in Slack in #core-customize by dlh. View the logs.
7 years ago
#7
@
7 years ago
In 40527.2.diff:
- Remove the cached copy of changeset data inside
WP_Customize_Manager
to address comment:3.
- Return
null
when instantiating a changeset for an invalid post.
- Remove a duplicate call to
json_last_error()
, and other minor fixes.
Although comment:3 mentions using the clean_cache_post
hook to reset the WP_Customize_Manager::_changeset_data
property, we also had discussed removing the cached value entirely in the REST API endpoints repo: https://github.com/WP-API/wp-api-customize-endpoints/pull/5#discussion_r118804446.
I went with the latter in this patch for its comparative simplicity and because core itself almost always calls WP_Customize_Manager::get_changeset_post_data()
, and that already bypasses the property.
Regarding the change to WP_Customize_Changeset::from_post()
: Returning null
would ensure that the two named constructors will return only changeset instances that contain at least a UUID so that WP_Customize_Changeset::save()
can be called reliably.
Returning null
also matches the behavior of get_post()
, and it removes the appearance of having retrieved a changeset by post ID when the lookup actually failed.
Still, one alternative to returning null
would be to still return a WP_Customize_Changeset
instance as happened in 40527.diff, but then return a WP_Error
early in WP_Customize_Changeset::save()
if the instance doesn't have a post ID or UUID.
#8
@
7 years ago
- Priority changed from normal to high
Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.
Yes. As mentioned previously, the
WP_Customize_Manager
class needs to be broken up into smaller classes.