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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

Issue tags: +Twig

tagging. 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.

joelpittet’s picture

Priority: Normal » Major

I see the problem, |raw doesn't seem to fix it so not sure if autoescape has much to do with it at this point.

joelpittet’s picture

Turned caching on so I could see the twig generated files.

Here is the output from D8 HEAD:

        echo twig_drupal_escape_filter($this->env, twig_round((1 / 4), 2, "floor"), "html", null, true);
        echo twig_drupal_escape_filter($this->env, twig_round((1 / 4), 1, "floor"), "html", null, true);
        echo twig_drupal_escape_filter($this->env, twig_round((1 / 4), 0, "floor"), "html", null, true);

Here is from straight up Twig 1.16

        echo twig_escape_filter($this->env, twig_round((1 / 4), 2, "floor"), "html", null, true);
        echo twig_escape_filter($this->env, twig_round((1 / 4), 1, "floor"), "html", null, true);
        echo twig_escape_filter($this->env, twig_round((1 / 4), 0, "floor"), "html", null, true);

So I'm going to try to debug/breakpoint what is happening in twig_drupal_escape_filter().

joelpittet’s picture

PHP is messing with me a bit... maybe someone can explain?

joelpittet’s picture

Here's some fun Minimum Viable Code.

test.php in your drupal root.

$twig = \Drupal::service('twig');
$env = new \Twig_Environment();
echo "\nA twig_drupal_escape_filter: '" . twig_drupal_escape_filter($env, twig_round((1 / 4), 0, "floor"), "html", null, true) . "'";
echo "\nB twig_escape_filter: '" . twig_escape_filter($env, twig_round((1 / 4), 0, "floor"), "html", null, true) . "'";

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.

joelpittet’s picture

Issue tags: +Autoescape, +Needs tests

Ok 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?

joelpittet’s picture

Here is a solution, give it a try and if it works maybe you can help me write a test for this @martin107?

joelpittet’s picture

Status: Active » Needs review
martin107’s picture

Assigned: Unassigned » martin107

@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...

joelpittet’s picture

Thanks @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.

joelpittet’s picture

martin107’s picture

Assigned: martin107 » Unassigned
Issue summary: View changes
Related issues: +#1825952: Turn on twig autoescape by default
FileSize
2.52 KB

Using 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.

joelpittet’s picture

Status: Needs review » Needs work

It'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.

joelpittet’s picture

Test 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

martin107’s picture

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.17 KB
1.97 KB
2.88 KB

Here'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!

joelpittet’s picture

There 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.

The last submitted patch, 16: drupal_8_breaks_twig_s-2417733-16-tests-only-fail.patch, failed testing.

taslett’s picture

Instead 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.

joelpittet’s picture

@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.

taslett’s picture

@joelpittet thanks for pointing that out, sounded like a clean solutiion for a moment.

joelpittet’s picture

Your in good company I thought that too and mikeker in irc mentioned that.

martin107’s picture

Thanks @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.

star-szr’s picture

Title: Drupal 8 breaks Twig's round filter. » Drupal 8 breaks Twig's round filter.
Status: Needs review » Needs work
+++ b/core/tests/Drupal/Tests/Core/Theme/TwigEngineTest.php
@@ -0,0 +1,59 @@
+ * Test covergage for the file core/themes/engines/twig/twig.engine.

s/covergage/coverage/

Other than that minor detail this is looking good to me. Anything else I'm missing?

joelpittet’s picture

Issue tags: +Novice
martin107’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
495 bytes

Thanks corrected.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

Looks good, has tests! Thank you :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed 852b0ba on 8.0.x
    Issue #2417733 by joelpittet, martin107: Drupal 8 breaks Twig's round...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.