Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Twig 1.x interprets whitespace differently under PHP 7.4 #3248

Closed
ctrlcctrlv opened this issue Jan 21, 2020 · 17 comments
Closed

Twig 1.x interprets whitespace differently under PHP 7.4 #3248

ctrlcctrlv opened this issue Jan 21, 2020 · 17 comments

Comments

@ctrlcctrlv
Copy link

See vichan-devel/vichan#366 and vichan-devel/vichan#367

I wonder, is this a known issue?

@stof
Copy link
Member

stof commented Jan 21, 2020

The template you use seems to have {% trans %} tag, and to expect the space to be part of the translation (there is no space between {% endtrans %} and omitted in the snippet in vichan-devel/vichan#367

Whichi implementation of {% trans %} are you using ? And how does your translation file looks like ?

@ctrlcctrlv
Copy link
Author

It might be easier to constrain the problem to vichan-devel/vichan#366, which was closed by vichan-devel/vichan@01538ed, though I suspect vichan-devel/vichan#367 has the same root cause. The entire templates/post/name.html doesn't contain a single trans tag.

However, to answer your question lest I appear defensive, we use php-gettext, a pure PHP gettext implementation, and then regular PoEdit-compatible gettext files.

@stof
Copy link
Member

stof commented Jan 21, 2020

which exact version of Twig are you using ?

@ctrlcctrlv
Copy link
Author

https://github.com/vichan-devel/vichan/blob/0aa4e3badc346aba27edbaa71da5966642ab060a/inc/lib/Twig/Environment.php#L19

    const VERSION = '1.35.4-DEV';

Sorry for the delay, took me a bit to figure out how to answer that definitively. We use all vendorized dependencies (our software predates Composer by two years).

@ctrlcctrlv
Copy link
Author

I suppose you would have me update Twig to the latest version in the 1.x branch and report back, yes? If that's a hard requirement for getting help I'll do it but the update is not easy, since, as I said, we use all vendorized dependencies with our own patches applied on top.

@ctrlcctrlv
Copy link
Author

(The patches are all in the Extensions/ directory.)

@xabbuh
Copy link
Contributor

xabbuh commented Jan 21, 2020

Are you sure that the behaviour changes are not in fact related to your patches? Can you reproduce the same with an unpatched version of Twig?

@stof
Copy link
Member

stof commented Jan 21, 2020

btw, why are you using your own patches if they are about registering extensions ? Twig does not require extensions to be part of Twig itself (quite the opposite).

@ctrlcctrlv
Copy link
Author

I hear you guys. I am preparing a minimal example right now. I just need to install PHP 7.3 alongside PHP 7.4 to demonstrate the difference in output. That way you don't need to worry about our hackery.

@ctrlcctrlv
Copy link
Author

OK friends, here you go. Observe:

[fred@pc twig-issue3248]$ php73 issue3248.php 
<span class="name">
[fred@pc twig-issue3248]$ php issue3248.php 
<spanclass="name">
[fred@pc twig-issue3248]$ php --version
PHP 7.4.1 (cli) (built: Dec 18 2019 12:59:28) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies

composer.json

{
    "require": {
        "twig/twig": "1.35.4"
    }
}

issue3248.php

<?php
require_once __DIR__ . '/vendor/autoload.php';

$loader = new \Twig\Loader\ArrayLoader([
    'index' => "<span {% if name.boo %}{{name.name}} {% endif %}class=\"name\">\n",
]);
$twig = new \Twig\Environment($loader);

echo $twig->render('index', ['name' => array("name" => 'Fabien')]);
?>

@xabbuh
Copy link
Contributor

xabbuh commented Jan 21, 2020

I can confirm the issue with your example on Twig 1.35.4. But when changing the version constraint to ~1.35 and then updating Twig (resulting in 1.42.4 being installed) the error is gone. Thus, I don't think there is anything to do here, but you need to update the Twig version that you use in your project.

@ctrlcctrlv
Copy link
Author

@xabbuh I understand. Thank you for your time. I don't know when I'll get a chance to do that. As I see it, the problem is in the interpreter. But they don't like to hear that.

@stof
Copy link
Member

stof commented Jan 21, 2020

well, #3004 contains a fix for the compatibility of Twig with PHP 7.4. That's why using the latest Twig version is important when reporting issues (fixes will always be based on top of the latest version anyway, so upgrading would also be necessary to get the fixes).

I'm closing this issue as it is already fixed in the latest 1.x version.

@stof stof closed this as completed Jan 21, 2020
@ctrlcctrlv
Copy link
Author

This issue will at least help Googlers, and serve as yet further evidence of the supremely bad decision making being made at the interpreter level. Mission accomplished if you ask me. I'll either cherry pick #3004 on top of 1.35.4, or do an upgrade to 1.4.

@kwisatz
Copy link

kwisatz commented Oct 21, 2020

Just wanted to note that for some reason, upgrading to 1.43.1 didn't solve this issue.
I see #3004 applied to my version of the twig source, yet it still rtrims whitespace before a {{.

Applying the minimal example from above to my case, I do get the correct behavior, but in the context of my entire code base, the behavior changes. I don't have enough time right now to follow this up further, I'll revert back to using PHP 7.3 on that specific project and investigate later.

My minimal example (which works with php7.4-cli, but not php7.4-fpm:

<?php
require_once __DIR__ . '/vendor/autoload.php';

$loader = new \Twig\Loader\ArrayLoader([
            'index' => '<span class="selector {{ category.slug }}{% if category.slug in selected %} active{% endif %}" data-selector="category" data-category="{{ category.slug }}">',
    ]);
$twig = new \Twig\Environment($loader);

echo $twig->render('index', ['category' => array("slug" => 'ec-1-lb'), 'selected' => []]);
?>

For cli, this produces

# php7.4 minimal.php 
<span class="selector ec-1-lb" data-selector="category" data-category="ec-1-lb">

and for fpm, I'm still seeing this in the generated PHP file:

 foreach ($context['_seq'] as $context["_key"] => $context["category"]) {
            // line 13
            echo "        <span class=\"selector";
            echo $this->getAttribute($context["category"], "slug", array());

I have restarted php7.4-fpm, I have cleared Twig and nginx caches. Any changes I make to the template are taken into consideration, but the rtrim here remain in effect. I know I'll facepalm when I find the actual issue here, but I can't think of anything else right now.

@stof
Copy link
Member

stof commented Oct 21, 2020

@kwisatz please open a new issue, providing a reproducing case (your comment does not provide such a case, as your snipped for the php-fpm generated file involves a loop and so it is definitely not the output of compiling your minimal example)

@kwisatz
Copy link

kwisatz commented Oct 21, 2020

No, it's not a valid minimal example for my problem, it was just applying the above example to my case (without the loop) and noticing that it's not reproducing the issue.

I'll open a new ticket when I get around to producing a valid minimal example that does recreate my issue. I was kinda hoping someone would point out the obvious oversight in what I did and that'd be it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants