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: Allow publishing from preview #115

Merged
merged 25 commits into from Aug 6, 2017

Conversation

Projects
None yet
3 participants
@miina
Copy link
Contributor

commented Feb 14, 2017

Resolves #103 .

@coveralls

This comment has been minimized.

Copy link

commented Feb 14, 2017

Coverage Status

Coverage decreased (-1.3%) to 75.635% when pulling 8270875 on feature/allow_publishing_from_preview into cd5a18f on develop.

}
request = wp.ajax.post( component.data.action, {
nonce: component.data.snapshotsFrontendPublishNonce,
uuid: component.data.uuid

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 14, 2017

Contributor

Should this also include theme? Would not a theme switch also be needing to be accounted for here?

This comment has been minimized.

Copy link
@miina

miina Feb 15, 2017

Author Contributor

Added.

One thought: maybe it would be useful to include the theme param into the preview link in Customizer in case a non-active theme is being customized? Otherwise I change the theme, customize it, click on preview icon, and end up previewing the active theme. Thoughts?

This comment has been minimized.

Copy link
@miina

miina Feb 15, 2017

Author Contributor

Ah, okay, just saw that this will basically be part of #101. Will pick that up.

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 18, 2017

Contributor

The only hesitation to including the theme in the preview link is that if a user is not authenticated they won't be able to access the preview, since previewing a theme switch requires the switch_themes capability.

);
wp_add_inline_script(
$handle,
sprintf( 'CustomizeSnapshotsFront.init( %s )', wp_json_encode( $exports ) ),

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 14, 2017

Contributor

👍

}
$wp_admin_bar->add_menu( array(
'id' => 'publish-customize-snapshot',
'title' => __( 'Publish Snapshot', 'customize-snapshots' ),

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 14, 2017

Contributor

Rename to “Publish Changeset”

$msg = __( 'Publishing failed: ', 'customize-snapshots' );
foreach ( $r->errors as $name => $value ) {
$msg .= $name . '; ';
}

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 14, 2017

Contributor

This could be replaced with: $msg .= join( "; ", array_keys( $r->errors ) ). But is the it the key or the values that have the error messages to share?

This comment has been minimized.

Copy link
@miina

miina Feb 15, 2017

Author Contributor

The keys seem to include the error messages.

return;
}
$this->current_snapshot_uuid = esc_attr( $_POST['uuid'] );

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 14, 2017

Contributor

Replace esc_attr( $_POST['uuid'] ) with sanitize_key( wp_unslash( $_POST['uuid'] ) ).

* These files control the behavior frontend.
*/
public function enqueue_frontend_scripts() {
if ( ! $this->snapshot ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 14, 2017

Contributor

This also needs to short-circuit if the current user cannot publish changesets. In other words:

if ( ! $this->snapshot || ! current_user_can( get_post_type_object( 'customize_changeset' )->cap->publish_posts ) ) {
* Publishes changeset from frontend.
*/
public function ajax_snapshot_frontend_publish() {
if ( ! check_ajax_referer( 'customize-snapshots-frontend-publish', 'nonce' ) ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 14, 2017

Contributor

Also important to check whether current_user_can( get_post_type_object( 'customize_changeset' )->cap->publish_posts )

@coveralls

This comment has been minimized.

Copy link

commented Feb 15, 2017

Coverage Status

Coverage decreased (-28.4%) to 48.507% when pulling f42969c on feature/allow_publishing_from_preview into cd5a18f on develop.

@coveralls

This comment has been minimized.

Copy link

commented Feb 15, 2017

Coverage Status

Coverage decreased (-1.6%) to 75.316% when pulling c5aaa1f on feature/allow_publishing_from_preview into cd5a18f on develop.

@miina

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2017

@westonruter The changes should be done, let me know if there's anything that still needs improving.

@coveralls

This comment has been minimized.

Copy link

commented Feb 20, 2017

Coverage Status

Coverage decreased (-1.6%) to 75.316% when pulling 93c91e1 on feature/allow_publishing_from_preview into cd5a18f on develop.

@westonruter westonruter added this to the Next Major Release milestone Mar 1, 2017

@westonruter

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

I'm waiting to do a final review and merge thus until 0.6.0 is released.

if ( ! window.confirm( component.data.confirmationMsg ) ) { // eslint-disable-line no-alert
return false;
}
if ( ! wp.customize.settings.theme.active ) {

This comment has been minimized.

Copy link
@miina

miina Jul 20, 2017

Author Contributor

@westonruter After merging develop there is an issue with publishing changeset from preview:
screen shot 2017-07-20 at 5 53 33 pm

Looks like wp.customize is not defined at this moment. Do you know if anything changed meanwhile in the logic or order of enqueuing the JS scripts? Tried to make the script dependent on customize-base but in this case wp.customize.settings is undefined.

Would be great if you could point out what to investigate or what might have changed to cause this issue. Not sure what I'm missing here, maybe it's something very obvious.

This comment has been minimized.

Copy link
@westonruter

westonruter Jul 20, 2017

Contributor

This wp.customize.settings value is actually set in customize-preview (and customize-controls) not customize-base.

See: https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-includes/js/customize-preview.js#L657-L658

Note that it is set at jQuery.ready.

But since you'd be clicking the button after jQuery.ready anyway, I don't see why you it would be undefined. Is customize-preview itself not getting enqueued? It should be if you've loaded a changeset on the frontend.

This comment has been minimized.

Copy link
@miina

miina Jul 27, 2017

Author Contributor

Seems like the issue was because I was using the param changeset_uuid=... instead of customize_changeset_uuid in frontend.

Note that customize-snapshots-frontend.js file was added to develop branch so I merged the frontend JS file from this branch (customize-snapshots-front.js) into the one in develop (customize-snapshots-frontend.js). Let me know if there are any concerns with that. Also added removing the UUID from storage after publishing.

There is another issue with publishing the changesets from frontend:
WP_Customize_Setting::_preview_filter is returning the dirty value as old value instead of the actual old value in DB meaning that the setting is not saved when publishing.

Although I can see that Post_Type::hooks() is adding this filter:
add_action( 'transition_post_status', array( $this, 'start_pretending_customize_save_ajax_action' ), $priority - 1, 3 );, this method is always returning true: WP_Customize_Setting::is_current_blog_previewed().

Looks like WP_Customize_Manager is initialized in _wp_customize_include() before the filters have any effect. Also, not sure if xwp/wordpress-develop#231 would fix that since the $_REQUEST['action'] is not customize_save at that moment.

Am I missing something or is creating another workaround necessary, e.g. reinitializing WP_Customize_Manager?

@westonruter

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

@miina:

Note that customize-snapshots-frontend.js file was added to develop branch so I merged the frontend JS file from this branch (customize-snapshots-front.js) into the one in develop (customize-snapshots-frontend.js). Let me know if there are any concerns with that. Also added removing the UUID from storage after publishing.

Yes, that's perfect.

There is another issue with publishing the changesets from frontend:
WP_Customize_Setting::_preview_filter is returning the dirty value as old value instead of the actual old value in DB meaning that the setting is not saved when publishing.

If I understand right, this is the exact same problem that you've patched for core, right? In other words: https://core.trac.wordpress.org/ticket/39221

The issue is that Ajax requests on the frontend get the customize_changeset_uuid injected into frontend requests: https://github.com/WordPress/wordpress-develop/blob/9f3bcacd713c23ed4f71e796f46090fb6376918f/src/wp-includes/js/customize-preview.js#L373-L442

So if you can prevent this from happening via another prefilter handler that removes the query parameter, then this would be one way to solve the problem I believe.

Another option, perhaps a better one, would be to eliminate the Ajax request entirely in favor of just linking to an admin URL that would be responsible for publishing the changeset. In other words, it could be a wp_nonce_url() for which has the necessary params for publishing the post, as if the user clicked the Publish button on the edit post screen. This would be a similar nonce URL that is shown for the Trash link in the post list table.

This would avoid having to try to override the customize_changeset_uuid from being added to the link, because any link in the admin bar is skipped from getting the customize_changeset_uuid injected into it: https://github.com/WordPress/wordpress-develop/blob/9f3bcacd713c23ed4f71e796f46090fb6376918f/src/wp-includes/js/customize-preview.js#L329-L335

@miina

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2017

@westonruter Reworked the logic to use wp_nonce_url instead of AJAX request, less hacky compared to pre-filtering query. Let me know if that looks something like you were thinking of.

@westonruter

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2017

@miina yes, that's it. I made a few additional changes in 4fd7832 but feel free to merge otherwise if it checks out for you.

@westonruter westonruter merged commit 3e8aefe into develop Aug 6, 2017

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/allow_publishing_from_preview branch Aug 6, 2017

@westonruter westonruter modified the milestones: Next Major Release, 0.7.0 Oct 30, 2017

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.