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

Remember preview url query params #129

Merged
merged 22 commits into from Mar 10, 2017

Conversation

Projects
None yet
3 participants
@sayedtaqui
Collaborator

sayedtaqui commented Mar 4, 2017

Fixes #107

@PatelUtkarsh PatelUtkarsh changed the title from [WIP]Rememeber preview url query params to [WIP]Remember preview url query params Mar 6, 2017

@coveralls

This comment has been minimized.

coveralls commented Mar 6, 2017

Coverage Status

Coverage decreased (-0.9%) to 75.339% when pulling 6a44a47 on enhancement/remember-preview-url into 973480f on develop.

@coveralls

This comment has been minimized.

coveralls commented Mar 6, 2017

Coverage Status

Coverage decreased (-1.04%) to 75.24% when pulling 95e139b on enhancement/remember-preview-url into 973480f on develop.

@sayedtaqui sayedtaqui changed the title from [WIP]Remember preview url query params to Remember preview url query params Mar 6, 2017

@sayedtaqui

This comment has been minimized.

Collaborator

sayedtaqui commented Mar 6, 2017

Ready for review.

I am not sure if preg_match( '/[a-z|\[|\]|_|-|0-9]+/', $value ) would be the correct choice for validating the panel/section/control id , please suggest

@coveralls

This comment has been minimized.

coveralls commented Mar 6, 2017

Coverage Status

Coverage decreased (-1.06%) to 75.226% when pulling 108c865 on enhancement/remember-preview-url into 973480f on develop.

@@ -226,6 +226,11 @@
api.previewer.query = function() {
var retval = originalQuery.apply( this, arguments );
if ( 'undefined' !== typeof CustomizerBrowserHistory ) {
retval.customize_preview_url_query_vars = JSON.stringify( CustomizerBrowserHistory.getQueryParams( location.href ) );

This comment has been minimized.

@westonruter

westonruter Mar 7, 2017

Contributor

I don't think that Customizer Browser History should be made a dependency for this feature.

This can be replaced with:

retval.customize_preview_url_query_vars = wp.customize.utils.parseQueryString( location.search.substr( 1 ) );

though on 4.6 a polyfill for wp.customize.utils.parseQueryString would be needed.

@@ -966,6 +966,11 @@
api.previewer.query = function() {
var retval = originalQuery.apply( this, arguments );
if ( 'undefined' !== typeof CustomizerBrowserHistory ) {
retval.customize_preview_url_query_vars = JSON.stringify( CustomizerBrowserHistory.getQueryParams( location.href ) );

This comment has been minimized.

@westonruter

westonruter Mar 7, 2017

Contributor

See above.

$allowed_devices = array(
'mobile',
'desktop',
'tablet',

This comment has been minimized.

@westonruter

westonruter Mar 7, 2017

Contributor

This list needn't be hard-coded. You can use \WP_Customize_Manager::get_previewable_devices().

(
in_array( $param, $allowed_panel_section_control_params )
&&
preg_match( '/[a-z|\[|\]|_|-|0-9]+/', $value )

This comment has been minimized.

@westonruter

westonruter Mar 7, 2017

Contributor

This regex seems to be missing the initial ^( and ending )$ if it is doing what I think.

This comment has been minimized.

@sayedtaqui

sayedtaqui Mar 8, 2017

Collaborator

The panel/sections/control ids can be like
post[post][1437]
title_tagline
blogname
nav_menu-61
postmeta[posttype][27450][keywords]

It doesn't follow any initial or ending pattern?

This comment has been minimized.

@westonruter

westonruter Mar 8, 2017

Contributor

Isn't the purpose to validate that the $param is a valid ID string? If you want to ensure that only the valid characters are included then you should do:

preg_match( '/^[a-z|\[|\]|_|-|0-9]+$/', $value )

Adding the ^ and $ anchors, as otherwise a user could supply an ID like: !@#$%^&*()title_tagline⁄€‹›fifl‡°·‚

This comment has been minimized.

@sayedtaqui

sayedtaqui Mar 8, 2017

Collaborator

@westonruter I think I am not able to get it, Can you see this http://regexr.com/3ffv4 ?

This comment has been minimized.

@sayedtaqui

sayedtaqui Mar 8, 2017

Collaborator

I should probably be \[a-z|\[|\]|_|\-|0-9]+\ ( escaped - sign ) but /^[a-z|\[|\]|_|-|0-9]+$/ doesn't match anything.

This comment has been minimized.

@westonruter

westonruter Mar 8, 2017

Contributor

@sayedtaqui from that example, you can see that it is matching all of those strings, the last one incorrect.

Compare:

image

and

image

This comment has been minimized.

@sayedtaqui

sayedtaqui Mar 9, 2017

Collaborator

got it.

@@ -104,7 +104,8 @@ function hooks() {
add_action( 'admin_bar_menu', array( $this, 'remove_all_non_snapshot_admin_bar_links' ), 100000 );
add_action( 'wp_before_admin_bar_render', array( $this, 'print_admin_bar_styles' ) );
add_filter( 'removable_query_args', array( $this, 'filter_removable_query_args' ) );
add_action( 'save_post_customize_changeset', array( $this, 'create_initial_changeset_revision' ) );
add_action( 'save_post_' . $this->get_post_type(), array( $this, 'create_initial_changeset_revision' ) );
add_action( 'save_post_' . $this->get_post_type() , array( $this, 'save_customize_preview_url_query_vars' ) );

This comment has been minimized.

@westonruter

westonruter Mar 7, 2017

Contributor

There's an extra space before the comment here.

$preview_url_query_vars = array();
// If customizer browser history plugin is active.
if ( function_exists( 'customizer_browser_history_enqueue_scripts' ) && $post_id ) {

This comment has been minimized.

@westonruter

westonruter Mar 7, 2017

Contributor

Per above, I think Customizer Browser History shouldn't be made a dependency.

@coveralls

This comment has been minimized.

coveralls commented Mar 8, 2017

Coverage Status

Coverage decreased (-0.9%) to 75.395% when pulling 1774137 on enhancement/remember-preview-url into 973480f on develop.

$preview_url_query_vars = array();
if ( $post_id ) {
$preview_url_query_vars = (array) get_post_meta( $post_id, '_preview_url_query_vars', true );

This comment has been minimized.

@westonruter

westonruter Mar 8, 2017

Contributor

If there is no postmeta on the changeset post, where get_post_meta() returns false, this will result in $preview_url_query_vars === array( false ).

So this should probably be:

$preview_url_query_vars = get_post_meta( $post_id, '_preview_url_query_vars', true );
if ( ! is_array( $preview_url_query_vars ) ) {
    $preview_url_query_vars = array();
}
@sayedtaqui

This comment has been minimized.

Collaborator

sayedtaqui commented Mar 9, 2017

Class 'PHPUnit_Framework_TestCase' not found in /tmp/wordpress/tests/phpunit/includes/testcase.

Not sure why the build is failing.

@westonruter

This comment has been minimized.

Contributor

westonruter commented Mar 9, 2017

Not sure why the build is failing.

@sayedtaqui it looks like an issue with PHPUnit 6.0. I'm noticing the same thing locally after upgrading from 5.x.

See also WordPress-Coding-Standards/WordPress-Coding-Standards#870 (comment)

See also:

Looks like travis-ci just updated its PHP 7 images to use PHPUnit 6+, so our PHP 7 builds are failing. We should either fast-track #39822 or install + use and older version of PHPUnit in our travis script
https://wordpress.slack.com/archives/core/p1489073667330796

@coveralls

This comment has been minimized.

coveralls commented Mar 10, 2017

Coverage Status

Coverage decreased (-0.8%) to 75.479% when pulling 83dc09f on enhancement/remember-preview-url into 973480f on develop.

@coveralls

This comment has been minimized.

coveralls commented Mar 10, 2017

Coverage Status

Coverage decreased (-0.8%) to 75.479% when pulling 4caa963 on enhancement/remember-preview-url into 973480f on develop.

@@ -104,7 +104,8 @@ function hooks() {
add_action( 'admin_bar_menu', array( $this, 'remove_all_non_snapshot_admin_bar_links' ), 100000 );
add_action( 'wp_before_admin_bar_render', array( $this, 'print_admin_bar_styles' ) );
add_filter( 'removable_query_args', array( $this, 'filter_removable_query_args' ) );
add_action( 'save_post_customize_changeset', array( $this, 'create_initial_changeset_revision' ) );
add_action( 'save_post_' . $this->get_post_type(), array( $this, 'create_initial_changeset_revision' ) );

This comment has been minimized.

@westonruter

westonruter Mar 10, 2017

Contributor

Good catch!

@westonruter westonruter added this to the 0.6.0 milestone Mar 10, 2017

@westonruter

This comment has been minimized.

Contributor

westonruter commented Mar 10, 2017

@sayedtaqui Ready for merge after you give a final review of my tweaks.

@coveralls

This comment has been minimized.

coveralls commented Mar 10, 2017

Coverage Status

Coverage decreased (-26.1%) to 50.176% when pulling f5b55cd on enhancement/remember-preview-url into 973480f on develop.

@coveralls

This comment has been minimized.

coveralls commented Mar 10, 2017

Coverage Status

Coverage decreased (-26.1%) to 50.176% when pulling 6d9e9a9 on enhancement/remember-preview-url into 973480f on develop.

@coveralls

This comment has been minimized.

coveralls commented Mar 10, 2017

Coverage Status

Coverage increased (+0.3%) to 76.554% when pulling 43bee5b on enhancement/remember-preview-url into 973480f on develop.

@sayedtaqui

This comment has been minimized.

Collaborator

sayedtaqui commented Mar 10, 2017

👍 Lets merge.

@westonruter westonruter merged commit 7f02238 into develop Mar 10, 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

@valendesigns valendesigns deleted the enhancement/remember-preview-url branch Aug 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment