WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#38987 closed defect (bug) (invalid)

Twenty Seventeen: Use of wrong function when escaping font url

Reported by: davidakennedy Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.7
Component: Bundled Theme Keywords: good-first-bug close
Focuses: Cc:

Description

See: https://wordpress.slack.com/archives/core-themes/p1480445730000676

/**
 * Register custom fonts.
 */
function twentyseventeen_fonts_url() {
    $fonts_url = '';

    /**
     * Translators: If there are characters in your language that are not
     * supported by Libre Franklin, translate this to 'off'. Do not translate
     * into your own language.
     */
    $libre_franklin = _x( 'on', 'Libre Franklin font: on or off', 'twentyseventeen' );

    if ( 'off' !== $libre_franklin ) {
        $font_families = array();

        $font_families[] = 'Libre Franklin:300,300i,400,400i,600,600i,800,800i';

        $query_args = array(
            'family' => urlencode( implode( '|', $font_families ) ),
            'subset' => urlencode( 'latin,latin-ext' ),
        );

        $fonts_url = add_query_arg( $query_args, 'https://fonts.googleapis.com/css' );
    }

    return esc_url_raw( $fonts_url );
}

return esc_url_raw( $fonts_url ); should be return esc_url( $fonts_url );

That function is more appropriate for the intended output here.

Change History (3)

#1 @ocean90
4 years ago

  • Keywords close added

I think the esc_url_raw() is appropriate here because twentyseventeen_fonts_url() only returns the URL, it doesn't print it. The value is currently passed to wp_enqueue_style() which uses esc_url() internally in WP_Styles::_css_href().

#2 @WPDevHQ
4 years ago

That makes sense to me now @ocean90 - thanks.

#3 @peterwilsoncc
4 years ago

  • Milestone 4.7 deleted
  • Resolution set to invalid
  • Status changed from new to closed

I agree with @ocean90 , 2012-'15 urlencode but do not escape the return value. As it stands, output is consistent across the default themes.

Note: See TracTickets for help on using tickets.