Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
So I have provided a patch - which demonstrates this failure.
{# outputs "0.25" as expected #}
<p>"{{(50/200)|round(2,'floor')}}"<p>
{# outputs "0.2" as expected #}
<p>"{{(50/200)|round(1,'floor')}}"</p>
{# outputs "", NOT "0" as expected #}
<p>"{{(50/200)|round(0,'floor')}}"</p>
As you can see rounding 0.25 down to 0 does not produce a zero, is does not produce any output at all.
My first action was to produce a bug report in Twig
https://github.com/twigphp/Twig/issues/1619
- the feeback was this link to twig fiddle http://twigfiddle.com/faj1rk
shows that Twig gets the value correct implying that somehow the d8 environment mangles the output.
Comment | File | Size | Author |
---|---|---|---|
#26 | interdiff-16-25.txt | 495 bytes | martin107 |
#26 | drupal_8_breaks_twig_s-2417733-25.patch | 2.88 KB | martin107 |
#16 | drupal_8_breaks_twig_s-2417733-16-pass.patch | 2.88 KB | joelpittet |
#16 | drupal_8_breaks_twig_s-2417733-16-tests-only-fail.patch | 1.97 KB | joelpittet |
#16 | interdiff.txt | 3.17 KB | joelpittet |
Comments
Comment #1
joelpittettagging. We mostly try not to modify twig. The only place I can think of this happening is our re-imaging of auto escape.
Haven't got my computer handy but if you have a chance try with auto escape off.
Comment #2
joelpittetI see the problem, |raw doesn't seem to fix it so not sure if autoescape has much to do with it at this point.
Comment #3
joelpittetTurned caching on so I could see the twig generated files.
Here is the output from D8 HEAD:
Here is from straight up Twig 1.16
So I'm going to try to debug/breakpoint what is happening in twig_drupal_escape_filter().
Comment #4
joelpittetPHP is messing with me a bit... maybe someone can explain?
Comment #5
joelpittetHere's some fun Minimum Viable Code.
test.php in your drupal root.
drush scr test.php
FYI the service call is so that twig is loaded by the class loader;) And I think it builds and loads the service container as well.
Comment #6
joelpittetOk the problem is that the twig_round() generates a 0 that is a double and a normal 0 is an int, and they don't === eachother.
The world starts to make sense again...
php -r 'var_dump((0 === (double) 0));'
I'm not sure the correct solution here. We can check for double 0 as well, or float 0?
Comment #7
joelpittetHere is a solution, give it a try and if it works maybe you can help me write a test for this @martin107?
Comment #8
joelpittetComment #9
martin107 CreditAttribution: martin107 commented@joelpittet, sure no problem, thanks for focusing on this ...
Its late here.. I will get to it tomorrow .. I am on european time.. so maybe by the time you wake up...
Comment #10
joelpittetThanks @martin107, we need to add this to twig_render_var() as well I just realized, that's why turning off escaping or using |raw didn't work either.
Comment #11
joelpittetComment #12
martin107 CreditAttribution: martin107 commentedUsing git blame I have provided a link to the issue which created the structure of twig_drupal_escape_filter()
My question revolves around why are we special casing zero ( 0 or now 0.0 ). Why is it special compare with 1, 2 or 3.0 ?
Is is being interpreted as FALSE by other parts of our code.
If so something smells bad ... Extraordinary code like this, should have extraordinary comments.
My first instinct was to remove the zero check at the start of the function ... and let the code fall through the is_scalar check and let it be subject to filtering like all other numbers but that does not work...
I also tried is_numeric() - TRUE/FALSE are not numeric so should be filtered out correctly, but no luck
We are in beta, so I don't want to touch this too much, but we should comment what is going on there.
My perspective, and others are welcome to have different opinions is that this is major only in that we need a fix and a regression test.
The refactoring and comments can be done is a normal task maybe even pushed to 8.1?
( if you feel that this line is too shallow and we should clean up the code now ... please let me know )
So here is my regression test, I hope it is clear from what I said above that something feels hacky...
To reduce the test to natural language here is what I have done
If the output of a twig filter is a zero ( in float or integer form) then for compatibility reasons the filtered response must be a '0' of type string.
I can see some other unrelated errors in the annotations of twig.engine .. and I am about to post another issue .. which I will link here.
PS I have altered twig_drupal_escape_filter() to return '0' - a string just to stay true to the stated return type of string|null
anyway hope this is all ok.
Comment #13
joelpittetIt's a performance thing from what I remember, exit early if we have a common/known/safe value. @Fabianx may be able to explain. is_numeric et al won't work but I think you came to that same conclusion above.
Can you post a tests only patch to show it fails as-is in head?
Also please don't alter to return '0', that could have some other consequences that we'd rather not have to fix later, 0 works.
Comment #14
joelpittetTest looks good to me:) I've not done as many unit tests but I like that it's bare minimum to test:)
Except for the '0' part, and maybe it should assert 0 returns 0 too.
The performance part is related to the part right after as we have many empty arrays or NULLs, but empty() is a bit heavy handed with 0 or '0' or 0.0
Comment #15
martin107 CreditAttribution: martin107 commentedComment #16
joelpittetHere's an iteration on the tests, needed to change assertEquals() to assertSame() to make sure the types are checked as well. FIxed some doc nit picks. Removed the details that is evident in the @see link to this node.
Added a test for twig_render_var() as well.
FYI there is a similar webtest called EngineTwigTest.php though I'm pretty happy this is a UnitTest, because they are way faster!
Comment #17
joelpittetThere is precedence for that require_once in the setUp() in almost all the tests in
core/modules/system/src/Tests/Update/
So I think we are safe there.
Comment #19
taslett CreditAttribution: taslett commentedInstead of
if ($arg === 0 || $arg === 0.0) {
could we use type casting
if ((int) $arg === 0 ) {
I'm not sure if this would be better performance wise just throwing in an idea.
Comment #20
joelpittet@taslett thanks for the suggestion but sorry that won't work.
Try this on the command line
php -r 'var_dump((int) "zero");'
You'll see what I mean, everything would be zero except real numbers.
Comment #21
taslett CreditAttribution: taslett commented@joelpittet thanks for pointing that out, sounded like a clean solutiion for a moment.
Comment #22
joelpittetYour in good company I thought that too and mikeker in irc mentioned that.
Comment #23
martin107 CreditAttribution: martin107 commentedThanks @joelpittet the tests look much better.
Thanks for answering all my questions and pointing out core/modules/system/src/Tests/Update/
Setting a precedent was something I was worried about.
Comment #24
star-szrs/covergage/coverage/
Other than that minor detail this is looking good to me. Anything else I'm missing?
Comment #25
joelpittetComment #26
martin107 CreditAttribution: martin107 commentedThanks corrected.
Comment #27
star-szrLooks good, has tests! Thank you :)
Comment #28
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 852b0ba and pushed to 8.0.x. Thanks!