[dependency evaluation] Adopt vincentlanglet/twig-cs-fixer for Twig coding standards

This will need a dependency evaluation → .

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Twig Coding Standard (twig-cs-fixer) found 120 errors, check attachment.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

MR !5248 adds vincentlanglet/twig-cs-fixer instead of friendsoftwig/twigcs about which is this issue.

That being said, friendsoftwig/twigcs maintainer is looking for someone to take over the maintainership and considers putting the project in the EOL status if no one will step up.
https://github.com/friendsoftwig/twigcs/issues/304

One alternative they point to is vincentlanglet/twig-cs-fixer (which MR !5248 adds).
Personally, I also prefer vincentlanglet/twig-cs-fixer since it also includes automatic fixes. We already using it for some time at work in Drupal based projects, and it works great.

Looks like `friendsoftwig/twigcs` will be EOL soon and `vincentlanglet/twig-cs-fixer` is a promising replacement.

Should we change the title and description to be more generic?

Yes, vincentlanglet/twig-cs-fixer is a tool with more adoption, more maintained and is more aligned with drupal releases in aspects like phpstan and following symfony releases.

I don't see the failure in the current PR but anyway it needs a rebase.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

#2 is still not addressed; this issue could be a whole lot of wasted effort if the dependency doesn't pass the dependency evaluation. I recommend ceasing rerolling/rebasing/etc. and instead completing the dependency evaluation, then tagging it "Needs release manager review" and "Needs frontend framework manager review".

Also things with lockfile changes are never good candidates for the friendly bot as they break constantly, and it's not worth rerolling something forever without other progress. So tagging so the bot leaves it alone. Thanks!

Took an initial stab at the dependency evaluation. Only thing that seems to be missing is the security policy – opened https://github.com/VincentLanglet/Twig-CS-Fixer/issues/255

Very nice work @Feuerwagen! Setting NR for review.

Thanks @Feuerwagen, as the maintainer is very responsive let's wait a few days for the security policy to hopefully be adopted and then we can move forward here.

Also fixing obsolete link in the IS.

My security policy proposal got adopted, see https://github.com/VincentLanglet/Twig-CS-Fixer/security/policy

Personally think the evaluation contains all good information.

Think this would be a welcome addition, have a current project I'm scared how much will get flagged.

I'm marking RTBC to get in front of the various managers :)

+1 for this.

I've started using this on a client site and had to customize it a bit for Drupal:

For spacing:

$ruleset->overrideRule(new TwigCsFixer\Rules\Whitespace\IndentRule(
    spaceRatio : 2,
    useTab : FALSE,
));

And for now I've had to exclude templates that use {% trans %} because it isn't defined in the default rules, and I couldn't get the ignore next line comments to work.

Evaluation looks good to me and as release manager I have no issues with adding this as a dev dependency.

However the MR currently has merge conflicts and also the package is added as a non-dev dependency so that needs to be fixed.

Conflicts are resolved, tests are passing, twig-cs-fixer is moved to require-dev. Seems like everything is addressed.

Per https://www.drupal.org/project/drupal/issues/3284817#comment-15729173 📌 Adopt friendsoftwig/twigcs for Twig coding standards Needs review , I have had the same issue with templates that utilize {% trans %} tags as well.

* The Symfony Twig documentation indicates that the proper syntax to use is

</code>.  This syntax validates.
* <a href="https://www.drupal.org/node/2047135">Drupal's documentation</a> indicates that the syntax to use for the {% trans %} tags is <code>{% trans %} Submitted by {{ author.name }} on {{ node.date }} {% endtrans %}

, which does not validate.

Which is correct?

If it is the former, before we adopt this we should correct our documentation, and IMO this is worth noting in the release notes.

TwigNodeTrans::compileString() calls trim() on the supplied text:

    return [
      new Node([new ConstantExpression(trim($text), $body->getTemplateLine())]),
      $tokens,
    ];

This means that whitespace either side of the text inside the tags is removed, so it doesn't matter which syntax we use.

Restoring to RTBC.

I am also getting

Error: Tag "trans" is already registered.
Failed to execute command vendor/bin/twig-cs-fixer lint --fix docroot/modules/custom --debug: exit status 2

@thejimbirch
could you please provide more information on when and how you got that error.

I just tested on TwigCSFixer 3.0.1, 3.0.3, 3.1.0 on D11.0.4 with and without $config->addTokenParser(new Drupal\Core\Template\TwigTransTokenParser()); on the following code

{% trans %} Submitted by {{ author.name }} on {{ node.date }} {% endtrans %}
{% trans %}Submitted by %author.name% on %node.date%{% endtrans %}

and it passed in all cases without issues.

@alex.skrypnyk, thanks for your testing. Since #27 when I had v2 installed, I've updated to v3 and retested. Now all the lines with trans pass. I can remove the folder exclusions that I had as a temporary workaround.

+1 on linting twig files and on the specific tool considered.

Merge conflict in the MR + a few comments.