Skip to content

Twig\Extension\CoreExtension::setEscaper() is deprecated#2070

Merged
gchtr merged 6 commits intotimber:2.xfrom
romainmenke:twig-extension-core-extension-set-escape-deprecated
Sep 26, 2019
Merged

Twig\Extension\CoreExtension::setEscaper() is deprecated#2070
gchtr merged 6 commits intotimber:2.xfrom
romainmenke:twig-extension-core-extension-set-escape-deprecated

Conversation

@romainmenke
Copy link
Copy Markdown
Contributor

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 22, 2019

Codecov Report

Merging #2070 into 2.x will decrease coverage by 0.11%.
The diff coverage is 78.26%.

Impacted file tree graph

@@             Coverage Diff             @@
##               2.x    #2070      +/-   ##
===========================================
- Coverage     95.8%   95.68%   -0.12%     
  Complexity    1517     1517              
===========================================
  Files           48       48              
  Lines         3862     3873      +11     
===========================================
+ Hits          3700     3706       +6     
- Misses         162      167       +5
Impacted Files Coverage Δ Complexity Δ
lib/Twig.php 96.19% <78.26%> (-2.65%) 53 <4> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56b2c35...9628f55. Read the comment docs.

@jarednova
Copy link
Copy Markdown
Member

This looks good to me @romainmenke! I had thought about getting a little more cleaver with assigning the class ('Twig\Extension\EscaperExtension' or 'Twig\Extension\CoreExtension') to a variable. While it seemed more graceful at first, I can see it being a pain to manage over time (i.e. if methods start to take different args, etc.). I think your solution is actually the simplest/clearest. @gchtr, what's your take?

Copy link
Copy Markdown
Member

@gchtr gchtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @jarednova that here, it’s probably better to not get too clever. The only thing I see that could be optimized is that we could save the anonymous functions in variables and reuse them:

$esc_url = function( \Twig\Environment $env, $string ) {
    return esc_url( $string );
}

if ( class_exists( 'Twig\Extension\EscaperExtension' ) ) {
	$twig->getExtension( 'Twig\Extension\EscaperExtension' )->setEscaper('esc_url', $esc_url );
} else {
    $twig->getExtension( 'Twig\Extension\CoreExtension' )->setEscaper( 'esc_url', $esc_url );
}

@romainmenke
Copy link
Copy Markdown
Contributor Author

romainmenke commented Sep 26, 2019

Reusing those makes sense. I also assigned the escaper extensions to a variable as 4 calls to get the same thing wasn't optimal either.

$escaper_extension = $twig->getExtension('Twig\Extension\EscaperExtension');
$escaper_extension->setEscaper('esc_url', $esc_url);
$escaper_extension->setEscaper('wp_kses_post', $esc_url);
$escaper_extension->setEscaper('esc_html', $esc_url);
$escaper_extension->setEscaper('esc_js', $esc_url);

Copy link
Copy Markdown
Member

@gchtr gchtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romainmenke Cool! Thanks for the changes. I think this looks much cleaner now.

However, I was merely using $esc_url as an example. Instead of using $esc_url for all escapers, we need to set variables for all the different anonymous functions. Sorry if that wasn’t clear enough!

$esc_url = function( \Twig\Environment $env, $string ) {
	return esc_url( $string );
}

$wp_kses_post = function( \Twig\Environment $env, $string ) {
    return wp_kses_post( $string );
}

$esc_html = function( \Twig\Environment $env, $string ) {
    return esc_html( $string );
}

$esc_js = function( \Twig\Environment $env, $string ) {
    return esc_js( $string );
}

And then:

$escaper_extension->setEscaper( 'esc_url', $esc_url );
$escaper_extension->setEscaper( 'wp_kses_post', $wp_kses_post );
$escaper_extension->setEscaper( 'esc_html', $esc_html );
$escaper_extension->setEscaper( 'esc_js', $esc_js );

@romainmenke
Copy link
Copy Markdown
Contributor Author

romainmenke commented Sep 26, 2019

However, I was merely using $esc_url as an example.

Yeah still early over here. Realised it just now too :)

@romainmenke
Copy link
Copy Markdown
Contributor Author

@gchtr should be better now. Thanks for the fast feedback!

Copy link
Copy Markdown
Member

@gchtr gchtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks for the updates! 👍

@gchtr gchtr self-assigned this Sep 26, 2019
@gchtr gchtr merged commit 5ef8472 into timber:2.x Sep 26, 2019
@gchtr
Copy link
Copy Markdown
Member

gchtr commented Sep 26, 2019

@romainmenke Congratulations on your first contribution to Timber 🍾 !

@romainmenke romainmenke deleted the twig-extension-core-extension-set-escape-deprecated branch September 26, 2019 17:02
jarednova added a commit that referenced this pull request Oct 2, 2019
jarednova added a commit that referenced this pull request Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants