osompress / genesis-portfolio-pro

Plugin: Genesis Portfolio Pro
24 stars 14 forks source link

layout option for portfolio-type term does not work with WP 4.4+ #18

Closed NicktheGeek closed 7 years ago

NicktheGeek commented 7 years ago

WP 4.4 introduced term meta so Genesis switched to support the new term meta logic. The layout setting check for the term meta is currently using the old method of checking the term meta setting. This needs to be updated to use the get_term_meta() function

dreamwhisper commented 7 years ago

Genesis Portfolio Pro

nickcernis commented 7 years ago

This change seems to have broken the layout of Portfolio Type archive pages.

Master branch

(body class always contains 'full-width-content', regardless of the Type's layout setting):

portfolio_types_flowers

release/1.1 branch

(body class contains no genesis layout classes regardless of Type layout settings):

portfolio_types_flowers edit_portfolio_type_ _wp_test_ _wordpress theme_settings_ _wp_test_ _wordpress

Cause

$layout always gets a value of 'true' instead of the expected layout string, due to the assignment of $opt within the ternary:

$term   = $wp_query->get_queried_object();
$layout = $term && isset( $term->term_id ) && $opt = get_term_meta( $term->term_id, 'layout', true ) ? $opt : __genesis_return_full_width_content();

Suggested fix

Any of these options fix the layout issue:

$term = $wp_query->get_queried_object();
$layout = $term && isset( $term->term_id ) && ( $opt = get_term_meta( $term->term_id, 'layout', true ) ) ? $opt : __genesis_return_full_width_content();
$term = $wp_query->get_queried_object();
$opt = get_term_meta( $term->term_id, 'layout', true );
$layout = $term && isset( $term->term_id ) && $opt ? $opt : __genesis_return_full_width_content();
$term = $wp_query->get_queried_object();
if ( $term && isset( $term->term_id ) ) {
    $taxonomy_layout = get_term_meta( $term->term_id, 'layout', true );
}

return $taxonomy_layout && ! is_null( $taxonomy_layout ) ? $taxonomy_layout : 'full-width-content';

Other notes

Also noting the label on the Type settings for the default layout option is not quite accurate:

edit_portfolio_type_ _wp_test_ _wordpress

Selecting that does not use the site's default layout; it uses a full-width layout.

Haven't yet checked if we can filter that, but perhaps use another label if we can: “Default full-width layout for portfolio archives.”

NicktheGeek commented 7 years ago

@nickcernis opted for putting a bracket around the ternary operation. The reason I'm running it this way is if the function to get the term object failed then the term_id wouldn't be available. The checks preceding setting the option variable make sure that it doesn't generate errors when it gets to that point. If it fails any of those is should use the default and call it a day, so for simplicity in the code it can/should be part of the ternary.

The brackets prevent it from getting the wrong assignment by picking up "true" or "false" instead of the ternary assigned values. It's interesting that there must be a difference in PHP handling this because when I tested this it worked correctly. So if this doesn't solve the issue in the environment you are testing in, I'll need to use something less streamlined. If it does fail can you let me know the version of PHP you have running?

nickcernis commented 7 years ago

Confirmed: the layout now works. Thanks!

there must be a difference in PHP handling this because when I tested this it worked correctly.

WP/PHP Coding Standards flag assignments within if statements or ternaries. It could be for the reason you highlight there (and because it's harder to read).

taxonomy-portfolio-type_php_ _wptest

(Screenshot from VS Code with the phpcs extension.)