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

feat: add rule to make sure that raw filter is justified by a comment #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions src/RawFilterRule.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
<?php

namespace Factorial\twigcs;

use FriendsOfTwig\Twigcs\Lexer;
use FriendsOfTwig\Twigcs\Rule\AbstractRule;
use FriendsOfTwig\Twigcs\Rule\RuleInterface;
use FriendsOfTwig\Twigcs\TwigPort\Token;
use FriendsOfTwig\Twigcs\TwigPort\TokenStream;

/**
* Custom Twigcs rule to make sure that "raw" filter is justified by a comment.
*/
class RawFilterRule extends AbstractRule implements RuleInterface {

/**
* {@inheritdoc}
*/
public function check(TokenStream $tokens): array {

$violations = [];

while (!$tokens->isEOF()) {
if ($tokens->isEOF()) {
break;
}
$token = $tokens->getCurrent();
if ($new_violations = $this->validateRawFilterComment($token, $tokens)) {
$violations = array_merge($violations, $new_violations);
}
$tokens->next();
}

return $violations;
}

/**
* Validates that any raw filter is preceded by a comment.
*/
protected function validateRawFilterComment(Token $token, TokenStream &$tokens): array {

$violations = [];

// Consider "|raw" filter.
if ($token->getValue() === 'raw' &&
Token::NAME_TYPE === $token->getType() &&
Token::PUNCTUATION_TYPE === $tokens->look(Lexer::PREVIOUS_TOKEN)->getType()
) {

$line = $token->getLine();
$column = $token->getColumn() + 1;
$reason = 'usage of the "raw" filter should be justified in a preceding comment.';

// If the "raw" filter appears on the first line there can not be a
// preceding comment.
if ($line === 1) {
$violations[] = $this->createViolation($tokens->getSourceContext()->getPath(), $line, $column, $reason);
return $violations;
}

// Look behind until the previous line is reached.
$look_behind = Lexer::PREVIOUS_TOKEN;
while ($previous_token = $tokens->look($look_behind)) {
if ($previous_token->getLine() !== $line) {
break;
}
$look_behind--;
}

// Check whether the previous line is a comment.
if ($previous_token->getType() !== Token::COMMENT_TYPE) {
$violations[] = $this->createViolation($tokens->getSourceContext()->getPath(), $line, $column, $reason);
}
}

return $violations;
}

}
3 changes: 2 additions & 1 deletion src/TwigCsRuleset.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ class TwigCsRuleset extends Official {
/**
* @{inheritdoc}
*/
public function getRules() {
public function getRules(): array {
$rules = parent::getRules();
return array_merge($rules, [
new WithOnlyRule(Violation::SEVERITY_ERROR),
new RawFilterRule(Violation::SEVERITY_WARNING),
]);
}

Expand Down
63 changes: 63 additions & 0 deletions tests/RulesetFunctionalTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php

namespace FriendsOfTwig\Twigcs\Tests;

use Factorial\twigcs\TwigCsRuleset;
use FriendsOfTwig\Twigcs\Lexer;
use FriendsOfTwig\Twigcs\Rule\RegEngineRule;
use FriendsOfTwig\Twigcs\TwigPort\Source;
use FriendsOfTwig\Twigcs\Validator\Validator;
use PHPUnit\Framework\TestCase;

/**
* Provides tests for the custom Twigcs ruleset.
*
* @author D34dMan <shibinkidd@gmail.com>
*/
class RulesetFunctionalTest extends TestCase {

/**
* @dataProvider getData
*/
public function testExpressions($expression, $violationCount): void {
$lexer = new Lexer();
$validator = new Validator();

$violations = $validator->validate(new TwigCsRuleset(2), $lexer->tokenize(new Source($expression, 'src', 'src.html.twig')));
$this->assertCount(0, $validator->getCollectedData()[RegEngineRule::class]['unrecognized_expressions'] ?? []);

if ($violationCount) {
$this->assertCount($violationCount, $violations, sprintf("There should be %d violation in:\n %s", $violationCount, $expression));
}
else {
$this->assertCount(0, $violations, sprintf("There should be no violations in:\n %s", $expression));
}
}

/**
* Provides test scenarios.
*/
public function getData(): array {
return [
// NOTE: We expect exactly 17 violations in the fail scenario.
// In case we add more, don't forget to update violation count.
[$this->getFailScenarios(), 17],
[$this->getPassScenarios(), 0],
Copy link
Member

Choose a reason for hiding this comment

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

@simonbaese ,

Do we have a bug due to this aggregation? E.g. false postive and false negative combination from different scenarios that nullify each other when aggregated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you give an example?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so because the lexer goes through the file token by token.

Copy link
Member

Choose a reason for hiding this comment

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

I am thinking about a scenario where a false positive is reported by the "with" rule and a false negative reported by "raw" rule which would cancel the aggregated count of each other.

];
}

/**
* Gets failure scenarios.
*/
public function getFailScenarios() {
return file_get_contents(__DIR__ . '/assets/fail.twig');
}

/**
* Gets pass scenarios.
*/
public function getPassScenarios() {
return file_get_contents(__DIR__ . '/assets/pass.twig');
}

}
52 changes: 0 additions & 52 deletions tests/WithOnlyRuleFunctionalTest.php

This file was deleted.

3 changes: 3 additions & 0 deletions tests/assets/fail.twig
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,7 @@
FAIL: {% include "@elements/icon/icon.twig" with {
only
} %}
FAIL: {{ content|raw }}
FAIL: {# Lorem ipsum dolor sit amet. #} {{ content|raw }}

</header>
9 changes: 9 additions & 0 deletions tests/assets/pass.twig
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,13 @@
PASS: {% include "@elements/icon/icon.twig" with {
only
} only %}
PASS:
{# Lorem ipsum dolor sit amet. #}
{{ content|raw }}
PASS:
{#
Lorem ipsum
dolor sit amet.
#}
{{ content|raw }}
</header>
Loading