Skip to content
This repository was archived by the owner on Mar 5, 2025. It is now read-only.

Issue 1543 twig lint replacement #2062

Closed

Conversation

wu-edward
Copy link

Fixes #1543.

Changes proposed:

  • Remove asm89/twig-lint and use Symfony twig linter
  • Run twig lint command procedurally instead of in parallel, otherwise error messages are suppressed.
  • Account for Twig functions and filters added by Drupal, so that linter does not flag as invalid.
  • Provide config for users to add custom Twig filters and functions that can be specified in project.yml

src/Robo/Blt.php Outdated
$twig = new Environment(new FilesystemLoader());

// Get any custom defined Twig filters to be ignored by linter.
$twig_filters = (array) $this->getConfig()->get('validate.twig.filters');
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, do you see an instance in which we will need to use this array? I see that it is currently empty.

Copy link
Author

Choose a reason for hiding this comment

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

If developers add Twig functions or filters to their custom modules and use them in their twig templates, they'll need to add them to their project.yml, otherwise they'll fail validation. I suppose the empty array can be left out of build.yml, and the ability to add functions and filters documented somewhere instead?

src/Robo/Blt.php Outdated
// Get any custom defined Twig filters to be ignored by linter.
$twig_filters = (array) $this->getConfig()->get('validate.twig.filters');
// Add Twig filters from Drupal TwigExtension to be ignored.
// @todo Find a way so that this list doesn't need to be maintained.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think this is where I started to lose the thread. I wanted to somehow load Drupal\Core\Template\TwigExtension into the $twig environment, but that led down a rabbit hole of determining how to get the right services to inject into TwigExtension.

src/Robo/Blt.php Outdated
$twig_filters = (array) $this->getConfig()->get('validate.twig.filters');
// Add Twig filters from Drupal TwigExtension to be ignored.
// @todo Find a way so that this list doesn't need to be maintained.
$drupal_filters = [
Copy link
Contributor

@grasmash grasmash Oct 2, 2017

Choose a reason for hiding this comment

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

Nobody loves regular expressions, but you could load the text content of docroot/core/lib/Drupal/Core/Template/TwigExtension.php and then use a regex to identify the functions and filters:

$matches_count = preg_match_all("#new \\Twig_SimpleFunction\('([^']+)',#", file_get_contents('docroot/core/lib/Drupal/Core/Template/TwigExtension.php'), $matches);
array_shift($matches);
$functions = $matches;
$matches_count = preg_match_all("#new \\Twig_SimpleFilter\('([^']+)',#", file_get_contents('docroot/core/lib/Drupal/Core/Template/TwigExtension.php'), $matches);
array_shift($matches);
$filters = $matches;

Copy link
Author

Choose a reason for hiding this comment

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

Oh, that's not a bad idea. I'll try to update the PR soon.

Copy link
Author

@wu-edward wu-edward Oct 3, 2017

Choose a reason for hiding this comment

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

Made the updates, and tested.
I'm not sure why Travis is failing, though.

@danepowell
Copy link
Contributor

I'm restarting the test to be sure, but it looks like something about this is causing the pre-commit validators to not run correctly.

@wu-edward
Copy link
Author

@danepowell I think you're right. I haven't had much luck reproducing locally, but I'll look into it more.

$filesets[$fileset_id] = $fileset_manager->filterFilesByFileset($files, $fileset);
}

$command = "'$bin/blt' 'validate:twig:files' '%s'";
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't you removing the validate:twig:files command above? Doesn't seem like it makes sense to call it here.

Copy link
Author

Choose a reason for hiding this comment

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

validate:twig:files gets added in src/Robo/Blt.php line 157.

@grasmash grasmash added the 9.x label Oct 10, 2017
@grasmash grasmash added this to the 9.1.0 milestone Oct 10, 2017
@wu-edward
Copy link
Author

Created new PR against 9.1 #2151

@wu-edward wu-edward closed this Oct 18, 2017
@wu-edward wu-edward deleted the issue-1543-twig-lint-replacement branch September 10, 2018 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants