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

Correctly handle case where 'post-thumbnails' is array of post types #7578

Merged
merged 4 commits into from Jun 28, 2018

Conversation

Projects
None yet
2 participants
@danielbachhuber
Member

danielbachhuber commented Jun 27, 2018

Description

Updates ThemeSupportCheck to support scenario where 'post-thumbnails' is an array of post types instead of a straight boolean value.

If 'post-thumbnails' is an array of post types, then postType becomes a required argument for PostFeaturedImageCheck (which is the only check referring to 'post-thumbnails' support.

Fixes #7547

How has this been tested?

  1. Add the following code snippet to your functions.php:
add_action( 'after_setup_theme', function(){
	remove_theme_support( 'post-thumbnails' );
	add_theme_support( 'post-thumbnails', array( 'post' ) );
}, 11 );
  1. Verify the "Featured Image" Post Setting is only present on the Post screen and not a Page screen.

Post:

image

Page:

image

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@danielbachhuber danielbachhuber added this to the 3.2 milestone Jun 27, 2018

@danielbachhuber danielbachhuber requested a review from WordPress/gutenberg-core Jun 27, 2018

return (
<ThemeSupportCheck supportKeys="post-thumbnails">
<ThemeSupportCheck supportKeys="post-thumbnails" postType={ postType } >

This comment has been minimized.

@aduth

aduth Jun 27, 2018

Member

Should it be the responsibility of the consumer to pass in the postType prop? Or should ThemeSupportCheck manage this itself?

Otherwise, how would we document what this prop is, and when it's to be passed? It's not really a "thing" so far as theme support goes. It's just one of many possible miscellaneous ...$args which could be passed.

My first inclination was: Maybe we don't have ThemeSupportCheck and we just make PostFeaturedImageCheck a simple series of higher-order components using withSelect (and getEditedPostAttribute, getThemeSupports, getPostTypeSupports) and ifCondition. But there is a point to be made about consolidating where we consider something like "post thumbnails is possibly an array".

Which then makes me wonder: Should this be part of a selector? Something like hasThemeSupport which includes as part of its internal logic to consider the key and any special considerations to make.

This comment has been minimized.

@danielbachhuber danielbachhuber requested a review from WordPress/gutenberg-core Jun 27, 2018

@aduth

aduth approved these changes Jun 28, 2018

// within `supported`. If `postType` isn't passed, then the check
// should fail.
if ( 'post-thumbnails' === key && isArray( supported ) ) {
if ( ! postType ) {

This comment has been minimized.

@aduth

aduth Jun 28, 2018

Member

In what (real-world) conditions do you imagine this condition would be satisfied?

Aside: If we keep it, we don't have a test case for it.

Aside 2: If it were the case that postType were undefined / falsey, wouldn't the includes logic still be sufficient to return false ?

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 28, 2018

Member

In what (real-world) conditions do you imagine this condition would be satisfied?

I added it to simply protect against some edge case where postType was defined but not to a specific string value.

Aside: If we keep it, we don't have a test case for it.

Aside 2: If it were the case that postType were undefined / falsey, wouldn't the includes logic still be sufficient to return false ?

Yes, it is. The conditional is more literal but save to remove. I've done so and added a test in 2ead29e

@danielbachhuber danielbachhuber merged commit ce657d2 into master Jun 28, 2018

2 checks passed

codecov/project 47.11% (+0.18%) compared to fffd8e3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber deleted the 7547-thumbnails-post-types branch Jun 28, 2018

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