WPBuddy / largo

A WordPress framework for news websites. Finely-crafted by INN and expertly-honed and maintained by the technology team at WP Buddy.
http://largo.wpbuddy.co
GNU General Public License v2.0
170 stars 83 forks source link

largo_render_template using previous $wp_query #1832

Open joshdarby opened 4 years ago

joshdarby commented 4 years ago

https://github.com/INN/largo/blob/512da701664b329f2f92244bbe54880a6e146431/inc/helpers.php#L244-L254

largo_render_template is using query vars from previous queries sometimes. Discovered because in the City Limits podcasts widget when $context has podcast => true, it doesn't get removed from the query when the query runs for the next widget below.

Updating

if ( ! empty( $context ) ) { 
     $context = apply_filters( 'largo_render_template_context', $context, $slug, $name ); 
     $wp_query->query_vars = array_merge( $wp_query->query_vars, $context ); 
   } 

to

if ( ! empty( $context ) ) { 
     $context = apply_filters( 'largo_render_template_context', $context, $slug, $name ); 
     $wp_query->query_vars = $context; 
   } 

works, but not sure of any implications of doing that.

benlk commented 4 years ago
if ( ! empty( $context ) ) {
    $context = apply_filters( 'largo_render_template_context', $context, $slug, $name ); 

    // we're about to modify $wp_query in hard-to-predict ways, so take a backup
    $_preserve = $wp_query;

    // get_template_part() uses locate_template() uses load_template(),
    // which extracts these variables into the context
    // @link https://developer.wordpress.org/reference/functions/load_template/
    $wp_query->query_vars = array_merge( $wp_query->query_vars, $context ); 
}

get_template_part( $slug, $name );

if ( ! empty( $context ) ) {
    // restore the backup
    $wp_query = $_preserve;
}
benlk commented 4 years ago

I'm not sure about that one, though, because the filter can change $context to be an empty() value. Are there any empty values of $context that would result in $wp_query->query_vars being modified by the array_merge?

benlk commented 4 years ago

This one doesn't reuse $context:

if ( ! empty( $context ) ) {
    $filtered_context = apply_filters( 'largo_render_template_context', $context, $slug, $name ); 

    // we're about to modify $wp_query in hard-to-predict ways, so take a backup
    $_preserve = $wp_query;

    // get_template_part() uses locate_template() uses load_template(),
    // which extracts these variables into the context
    // @link https://developer.wordpress.org/reference/functions/load_template/
    $wp_query->query_vars = array_merge( $wp_query->query_vars, $filtered_context );
}

get_template_part( $slug, $name );

if ( ! empty( $context ) ) {
    // restore the backup
    $wp_query = $_preserve;
}
benlk commented 4 years ago

Lesson learned: When mucking with $wp_query, reset it.

benlk commented 4 years ago

In WP 5.5, we can get rid of largo_render_template: https://make.wordpress.org/core/2020/07/17/passing-arguments-to-template-files-in-wordpress-5-5/