Yoast / wordpress-seo

Yoast SEO for WordPress
https://yoast.com/wordpress/plugins/seo/
Other
1.78k stars 894 forks source link

Breadcrumbs Title field not shown when theme support 'yoast-seo-breadcrumbs' #13015

Open mingyeungs opened 5 years ago

mingyeungs commented 5 years ago

Please give us a description of what happened.

When "Breadcrumbs" is set to disabled (which is the default setting btw), and the current theme has (add_theme_support( 'yoast-seo-breadcrumbs' );), the Breadcrumbs Title field (shown in green box below) for custom post types is not shown on the Search Appearance page.

yseo_search-appearance_template_cpt-archive

Please describe what you expected to happen and why.

The Breadcrumbs Title fields should show when the theme support 'yoast-seo-breadcrumbs', no matter if "Breadcrumb settings" has been explicitly set to Enabled.

Also notice that the "Enable Breadcrumbs" field is hidden when the theme support 'yoast-seo-breadcrumbs', and the user have no way to turn that on even if they wanted to.

Screen Shot 2019-05-23 at 4 32 22 PM

How can we reproduce this behavior?

  1. Fresh install Wordpress and Yoast SEO
  2. Activate a theme with add_theme_support( 'yoast-seo-breadcrumbs' )
  3. The breadcrumb title fields are not shown

Related code

Whether to show Breadcrumbs TItle is determined by WPSEO_Options::get( 'breadcrumbs-enable' ) === true (wordpress-seo/admin/views/tabs/metas/paper-content/post-type-content.php : Line 60).

The condition should be revised here, or WPSEO_Options::get( 'breadcrumbs-enable' ) should return true when theme support 'yoast-seo-breadcrumbs'

Technical info

Used versions

Djennez commented 5 years ago

I was able to reproduce this bug with v11.3-RC2 Premium and WP 5.2.1.

For development:

The check for this is here: https://github.com/Yoast/wordpress-seo/blob/f9e2cdddf1c529d6f517d6634bdbefc08355cf25/admin/views/tabs/metas/paper-content/post-type-content.php#L60-L64

Changing the conditional to this will make it work:

if ( WPSEO_Options::get( 'breadcrumbs-enable' ) === true || current_theme_supports( 'yoast-seo-breadcrumbs' )) {

However, I have a question about this: why are we treating these 2 conditionals separately? This would imply that somewhere down the line we expect different results if any of these conditionals differ. If this is just legacy, would it not be better to have checks for the theme support and override the option in the database to reflect this support? So that we can use just the WPSEO_Options::get( 'breadcrumbs-enable' anywhere instead of having to check for both (like in here).

jdevalk commented 5 years ago

I disagree that making it show there is the right solution: the fact that the theme supports it, doesn't mean you want to have them on. This looks like there is some logic that is weird though, will investigate.