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

Allow snapshot posts to publish and be scheduled #62

Merged
merged 39 commits into from Jul 27, 2016

Conversation

Projects
None yet
2 participants
@PatelUtkarsh
Copy link
Collaborator

commented Jul 20, 2016

This allows snapshot changes to be published, On publish it will save snapshot changes in database.

Fixes #15.

PatelUtkarsh added some commits Jul 20, 2016

@PatelUtkarsh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2016

Do we need to check for theme change and change theme before publishing snapshot data and revert back after done? @valendesigns

* @param int $post_id Post id.
* @param \WP_Post $post Post object.
*/
public function publish_snapshot( $post_id, $post ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 20, 2016

Contributor

@PatelUtkarsh warning that when doing Publish in the Customizer, any existing snapshot will be transitioned to publish. See:

So it is important that this should short-circuit if did_action( 'customize_save' ), because this would indicate that the customize_save Ajax request is being made.

foreach ( $snapshot_values as $setting_id => $setting_params ) {
$this->snapshot_manager->customize_manager->set_post_value( $setting_id, $setting_params );
$this->snapshot_manager->customize_manager->add_setting( $setting_id, $setting_params );

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 20, 2016

Contributor

@PatelUtkarsh this isn't going to work as expected. Instead, you should above do:

$this->snapshot_manager->customize_manager->add_dynamic_settings( array_keys( $snapshot_values ) );

This will ensure that the dynamic settings are added. For static settings, it will be important to do:

do_action( 'customize_register', $this->snapshot_manager->customize_manager );
require_once( ABSPATH . WPINC . '/class-wp-customize-manager.php' );
$this->snapshot_manager->customize_manager = new \WP_Customize_Manager();
}
$snapshot_content = json_decode( $post->post_content, true );

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 20, 2016

Contributor

Instead of accessing the post_content directly, it would be better to do:

$snapshot_content = $this->snapshot_manage->post_type->get_post_content( $post );
@westonruter

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2016

@PatelUtkarsh:

Do we need to check for theme change and change theme before publishing snapshot data and revert back after done?

I don't think we need to worry about the theme change. The publish metabox and publish actions should be disabled if the current theme doesn't match the theme associated with the snapshot.

@westonruter

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2016

@PatelUtkarsh the changes here will directly resolve #15. Please refer to the comments there for what other considerations will be needed.

foreach ( $snapshot_values as $setting_id => $setting_params ) {
$this->snapshot_manager->customize_manager->set_post_value( $setting_id, $setting_params );
$this->snapshot_manager->customize_manager->add_setting( $setting_id, $setting_params );
$setting = $this->snapshot_manager->customize_manager->get_setting( $setting_id );

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 20, 2016

Contributor

@PatelUtkarsh you'll need to make sure that $setting is not null. Also, as noted in #15, you'll need to make sure you do: $setting->capability = 'exist' to ensure that the setting save will always be allowed. This is safe because a setting only be written to the snapshot by a user who has the capability to begin with, so we know at this point it will be safe to save.

$this->snapshot_manager->customize_manager->add_setting( $setting_id, $setting_params );
$setting = $this->snapshot_manager->customize_manager->get_setting( $setting_id );
$setting->save();
}

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 20, 2016

Contributor

@PatelUtkarsh make sure you also do_action( 'customize_save_after', $this->snapshot_manager->customize_manager )

if ( ! is_a( $this->snapshot_manager->customize_manager, '\WP_Customize_Manager' ) ) {
require_once( ABSPATH . WPINC . '/class-wp-customize-manager.php' );
$this->snapshot_manager->customize_manager = new \WP_Customize_Manager();
}

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 20, 2016

Contributor

Add this:

if ( ! did_action( 'customize_register' ) ) {
    do_action( 'customize_register', $this->snapshot_manager->customize_manager );
}
function( $value ) {
return ! is_null( $value );
}
);

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 20, 2016

Contributor

You'll also want to do this here, to ensure all of the expected actions are done as found in WP_Customize_Manager::save():

if ( method_exists( $this->snapshot_manager->customize_manager, 'validate_setting_values' ) ) {
    do_action( 'customize_save_validation_before', $this->snapshot_manager->customize_manager );
}
@@ -154,7 +154,6 @@ public function setup_metaboxes() {
*/
public function remove_publish_metabox() {
remove_meta_box( 'slugdiv', static::SLUG, 'normal' );
remove_meta_box( 'submitdiv', static::SLUG, 'side' );

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 20, 2016

Contributor

Keep removing if the snapshot's associated theme is not equal to the current theme.

@PatelUtkarsh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 21, 2016

@westonruter Which post_status transition should we allow?

any => publish or 
only draft => publish
@westonruter

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

@PatelUtkarsh any transition to publish. Particularly, this is key for scheduling snapshots (#15), as it will allow you to create update a snapshot post with a future date and it will have future status. When the future date arrives, WP Cron should transition the post from future to publish and all of the Customizer settings in that post should get saved. (If the theme associated with the snapshot doesn't match the current theme, then we should probably short-circuit and change the status back to pending.)

PatelUtkarsh added some commits Jul 21, 2016

remove_action( 'customize_save', array( $this->snapshot_manager, 'check_customize_publish_authorization' ), 10 );
/** This action is documented in wp-includes/class-wp-customize-manager.php */
do_action( 'customize_save', $this->snapshot_manager->customize_manager );
add_action( 'customize_save', array( $this, 'check_customize_publish_authorization' ), 10, 0 );

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 22, 2016

Contributor

Looks like the wrong customize_save handler is being re-added. There is no check_customize_publish_authorization method on this class.

I suggest doing:

$action = 'customize_save';
$callback = array( $this->snapshot_manager, 'check_customize_publish_authorization' );
$priority = has_action( $action, $callback );
if ( false !== $priority ) {
    remove_action( $action, $callback, $priority );
}
/** This action is documented in wp-includes/class-wp-customize-manager.php */
do_action( 'customize_save', $this->snapshot_manager->customize_manager );
if ( false !== $priority ) {
    add_action( $action, $callback, $priority );
}
}
foreach ( $filtered_setting_objs as $setting_obj ) {
$setting_obj->capability = 'exist';

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 22, 2016

Contributor

We should probably capture what the original capability was before we override it, and then restore the original capability after we save them all. This will ensure that a setting doesn't accidentally get leaked out with blanket permissions.

}
$filtered_setting_objs = array_filter( $setting_objs, function( $setting_obj ) {
return is_a( $setting_obj, '\WP_Customize_Setting' );

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 22, 2016

Contributor

I think it's better to do return $setting_obj instanceof \WP_Customize_Setting. It's more modern.

return is_a( $setting_obj, '\WP_Customize_Setting' );
} );
if ( empty( $filtered_setting_objs ) ) {
return;

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 22, 2016

Contributor

I've been thinking about this, about the scenario where a setting no longer is recognized when a snapshot is published. At the moment, they just silently get skipped. But I think there should be a way to log when a setting fails to save for reasons such as the value no longer being valid or the setting not being recognized. We could introduce a save_error perhaps for each setting in a snapshot which gets set when the snapshot is saved, and this could potentially store such failure information.

return;
}
$snapshot_theme = get_post_meta( get_the_ID(), '_snapshot_theme', true );
if ( ! empty( $snapshot_theme ) && get_stylesheet() !== $snapshot_theme ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 22, 2016

Contributor

@valendesigns What were the reasons for preventing snapshots from being published when doing a theme switch? I can't recall. Is it because themes may define their own settings, and if a different theme is active when a snapshot is loaded or saved, it could result in there being unrecognized settings?

@westonruter

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2016

@PatelUtkarsh this commit may be especially relevant to you, specifically the date_gmt field: 13f8edf

See #63

PatelUtkarsh added some commits Jul 25, 2016

if ( false !== $priority ) {
remove_action( $action, $callback, $priority );
}
$this->snapshot_manager->customize_manager->doing_ajax( 'customize_save' );

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 25, 2016

Contributor

The doing_ajax method doesn't do anything other than return a bool to say whether this Ajax action is happening. So I don't think this block is doing anything. I'm revising.

westonruter added some commits Jul 25, 2016

[WIP] Add prepare_snapshot_post_content_for_publish
* Remove bulk actions filter
* Replace publish filter with transition filter.
* Remove hiding of publish metabox if not current theme. Allow publishing if not current theme.
* Remove hiding of author metabox.
* Remove extraneous info now that publish metabox is restored.
* Move Post_Type logic to Customize_Snapshot_Manager

@westonruter westonruter changed the title [WIP] Allow snapshot to publish Allow snapshot posts to publish Jul 26, 2016

@PatelUtkarsh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 27, 2016

@westonruter In a case of null as setting value before publishing snapshot, it is not showing setting name, and after publishing it shows setting name with an error.
Should we fix this and show null setting name before publishing?

@westonruter westonruter changed the title Allow snapshot posts to publish Allow snapshot posts to publish and be scheduled Jul 27, 2016

@westonruter westonruter merged commit 32c03e9 into feature/refactor Jul 27, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the feature/publish-snapshot branch Jul 27, 2016

@westonruter westonruter referenced this pull request Jul 27, 2016

Merged

Refactor #59

3 of 3 tasks complete

@westonruter westonruter modified the milestone: 0.5.0 Aug 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.