Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Conversation

@westonruter
Copy link
Contributor

@westonruter westonruter commented Apr 10, 2016

Still needed:

  • Demonstration of adding a control.
  • Add missing unit tests.
  • Sync page template selection when doing page preview.

Fixes #1.
Fixes #85.
Fixes #62.

Includes WP_Customize_Page_Template_Postmeta_Setting
…-dev-lib#164 from branch bugfix/linting-failures

xwp/wp-dev-lib@12991a3...05bc18b

59fc96e Let JSHint know about pre-defined global variables of Browserify and Qunit.
0f110d4 Merge pull request xwp/wp-dev-lib#163 from branch feature/jshint-browserify-qunit
f88ac9c Gracefully handle npm install failures
e8af3c0 Ensure JSHINT_IGNORE is relative to linting directory
05bc18b Merge pull request xwp/wp-dev-lib#164 from branch bugfix/linting-failures
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e9085a9 on feature/postmeta into * on develop*.

@westonruter westonruter changed the title [WIP] Add postmeta setting and framework Add postmeta setting and framework Apr 14, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7dbf83c on feature/postmeta into * on develop*.

@valendesigns
Copy link
Contributor

valendesigns commented Apr 15, 2016

@westonruter I'm a little confused about how WP_Customize_Page_Template_Controller works. From what I can tell the sanitize functions are not being used for the setting. I came across Test_Customize_Postmeta_Setting::test_sanitize_page_template_setting and noticed that the only way to get the test to pass was to explicitly add the sanitize_callback param in the args array when instantiating WP_Customize_Postmeta_Setting. Below is a passing test, though to me this demonstrates that the args are not being filtered because this should happen automatically.

function test_sanitize_page_template_setting() {
    switch_theme( 'twentytwelve' );

    $post_id = $this->factory()->post->create( array( 'post_type' => 'post') );
    $meta_key = '_wp_page_template';
    register_meta( 'post', $meta_key, array( $this->plugin->page_template_controller, 'sanitize_value' ) );
    $setting_id = WP_Customize_Postmeta_Setting::get_post_meta_setting_id( get_post( $post_id ), $meta_key );
    $setting = new WP_Customize_Postmeta_Setting( $this->manager, $setting_id, array( 
        'sanitize_callback' => array( $this->plugin->page_template_controller, 'sanitize_setting' )
    ) );

    $this->assertEquals( 'default', $setting->sanitize( 'default' ) );
    $this->assertNull( $setting->sanitize( 'bad-template.php' ) );
    $page_template = 'page-templates/front-page.php';
    $this->assertEquals( $page_template, $setting->sanitize( $page_template ) );
}

@westonruter
Copy link
Contributor Author

@valendesigns

I'm a little confused about how WP_Customize_Page_Template_Controller works

You can see committed right now that the test is skipped:

$this->markTestSkipped( 'Class is gone: WP_Customize_Page_Template_Postmeta_Setting' );

So it wasn't up to date.

Now, it is expected that it would fail without the sanitize_callback arg supplied. Why? Because this filter makes sure that the supplied template actually refers to an actual page template in the theme. So when WP_Customize_Page_Template_Controller::sanitize_setting() the method is applying, it is expecting that the sanitized value should be null. And this is what the assertion is checking. When the sanitize callback is not present, the unit test fails as expected:

Failed asserting that 'bad-template.php' is null.

There are actually two separate sanitizers that can apply to a postmeta setting. There is the only sanitizer that is added via register_meta():

register_meta( 'post', $meta_key, array( $this->plugin->page_template_controller, 'sanitize_value' ) );

This sanitizer is called when sanitize_meta() is called, and this is called in \WP_Customize_Postmeta_Setting::sanitize(). This method also calls apply-filters on customize_sanitize_{$this->id} which is what calls the sanitize_callback. Why do we have two separate sanitizers? Because when sanitize_meta() is called, it doesn't supply the context of the post for the postmeta being sanitized. When the Customizer setting sanitizer is called, it has access to the WP_Customize_Setting instance, and thus has access to the post ID and post type that form part of the setting ID. This will allow us to eventually use the setting sanitizer to enforce a very interesting, although infrequently-used feature: you can have different page templates made available to different pages via the theme_page_templates filter. And this is enforced now by passing in the $post object:

$page_templates = wp_get_theme()->get_page_templates( $post );

So yeah, I think what you found is expected behavior!

$meta_values = get_post_meta( $object_id, '', $single );
$is_recursing = false;

foreach ( $this->previewed_postmeta_settings[ $object_id ] as $postmeta_setting ) {
Copy link
Member

@mehigh mehigh Apr 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter
When testing I got this notice, and it was related to this line.

array(1) { ["postmeta[page][10][_wp_page_template]"]=> string(31) "template-page-sidebar-right.php" } string(37) "postmeta[page][10][_wp_page_template]"
Warning: array_key_exists() expects parameter 2 to be array, string given in /srv/www/strathcomcms.com/wp-content/plugins/wp-customize-posts/php/class-wp-customize-posts-preview.php on line 228

I checked http://php.net/manual/ro/function.array-key-exists.php
and this was mentioning to turn those things - they key first, the array after, like from this:
array_key_exists( $post_values, $postmeta_setting->id )
into this:
array_key_exists( $postmeta_setting->id, $post_values )

Fixed it here: 9cc9fe8

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9cc9fe8 on feature/postmeta into * on develop*.

setting.bind( function( pageTemplate ) {
var settings = {};
settings[ settingId ] = pageTemplate;
EditPostPreviewCustomize.sendSettingsToEditPostScreen( settings );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter
This EditPostPreviewCustomize global yielded an invalid reference. I've updated Wordpress to 4.5 in our test environment and didn't get an improvement.
I've searched for this variable and since it's not in the current branch of the repository I'm not sure where it originates from.
I've just commented this out (locally) and that yielded a fully functional page template adjustment without JS errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mehigh good catch! I've fixed this in f17aecb. The problem was that EditPostPreviewCustomize is only available in the context of opening the Customizer when in a post/page preview.

@mehigh
Copy link
Member

mehigh commented Apr 16, 2016

I had brought this into a client project, since it worked. We'll be coming back if we find anything not to be working correctly.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 562c068 on feature/postmeta into * on develop*.

…ve WP_Customize_Posts_Preview logic for returning non-single values
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 09077f7 on feature/postmeta into * on develop*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 87bd48b on feature/postmeta into * on develop*.

@westonruter
Copy link
Contributor Author

@valendesigns this epic PR I believe is now ready to merge!

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f17aecb on feature/postmeta into * on develop*.

@westonruter westonruter merged commit 76ce256 into develop Apr 17, 2016
@westonruter westonruter added this to the 0.5 milestone Apr 28, 2016
@westonruter westonruter deleted the feature/postmeta branch April 28, 2016 21:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants