Make WordPress Core

Opened 8 years ago

Last modified 7 years ago

#40527 new enhancement

Decouple WP_Customize_Manager

Reported by: utkarshpatel's profile 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)

40527.diff (20.5 KB) - added by dlh 7 years ago.
40527.2.diff (22.6 KB) - added by dlh 7 years ago.

Download all attachments as: .zip

Change History (12)

#1 @westonruter
8 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Yes. As mentioned previously, the WP_Customize_Manager class needs to be broken up into smaller classes.

#2 @westonruter
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 @westonruter
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

#4 @westonruter
7 years ago

  • Milestone changed from Future Release to 4.9

@dlh
7 years ago

#5 @dlh
7 years ago

40527.diff is a first attempt at a WP_Customize_Changeset class.

Some notes about the patch:

  • There is, of course, room to add more functionality from WP_Customize_Manager -- for instance, more of the logic from save_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

@dlh
7 years ago

#7 @dlh
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 @westonruter
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.

This ticket was mentioned in Slack in #core-customize by jeffpaul. View the logs.


7 years ago

#10 @westonruter
7 years ago

  • Milestone changed from 4.9 to Future Release
  • Priority changed from high to normal

We'll re-visit this, perhaps with Customizer v2 feature plugin during 5.0.

Note: See TracTickets for help on using tickets.