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

Add Twig template exists rule #405

Open
wants to merge 3 commits into
base: 1.4.x
Choose a base branch
from

Conversation

zacharylund
Copy link
Contributor

Fixes #257

@zacharylund zacharylund force-pushed the twig-template-exists-rule branch from d0a7ea1 to 396dc78 Compare August 21, 2024 13:23
Copy link
Contributor

@ruudk ruudk left a comment

Choose a reason for hiding this comment

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

I would use the Twig environment using a loader, then it auto picks up all namespaces.

extension.neon Show resolved Hide resolved
@zacharylund zacharylund force-pushed the twig-template-exists-rule branch from ba45518 to 9b97b95 Compare August 25, 2024 20:07
@zacharylund
Copy link
Contributor Author

I would use the Twig environment using a loader, then it auto picks up all namespaces.

@ruudk, I've updated this to use a loader, but I'm not loving it. Because twig is a private service, the loader would need to look something like:

use App\Kernel;

use App\Kernel;
use Symfony\Component\Dotenv\Dotenv;

require __DIR__ . '/../vendor/autoload.php';

(new Dotenv())->bootEnv(__DIR__ . '/../.env');

$kernel = new Kernel('test', (bool) $_SERVER['APP_DEBUG']);
$kernel->boot();

return $kernel->getContainer()->get('test.service_container')->get('twig');

@ruudk
Copy link
Contributor

ruudk commented Aug 26, 2024

Yes. Downside is that now you are using the test environment.

In TwigStan I came up with this solution:
https://github.com/twigstan/twigstan/blob/90e1792a68dc0cd9471ce87a1e1f0145467299f9/twig-loader-symfony.php.dist#L18-L26

Comment on lines +50 to +58
if ($templateArg->value instanceof Variable && is_string($templateArg->value->name)) {
$varType = $scope->getVariableType($templateArg->value->name);

foreach ($varType->getConstantStrings() as $constantString) {
$templateNames[] = $constantString->getValue();
}
} elseif ($templateArg->value instanceof String_) {
$templateNames[] = $templateArg->value->value;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can avoid the if/else and write

Suggested change
if ($templateArg->value instanceof Variable && is_string($templateArg->value->name)) {
$varType = $scope->getVariableType($templateArg->value->name);
foreach ($varType->getConstantStrings() as $constantString) {
$templateNames[] = $constantString->getValue();
}
} elseif ($templateArg->value instanceof String_) {
$templateNames[] = $templateArg->value->value;
}
$argType = $scope->getType($templateArg->value);
foreach ($varType->getConstantStrings() as $constantString) {
$templateNames[] = $constantString->getValue();
}

Comment on lines +66 to +75
foreach ($templateNames as $templateName) {
if ($this->twigEnvironmentResolver->templateExists($templateName)) {
continue;
}

$errors[] = RuleErrorBuilder::message(sprintf(
'Twig template "%s" does not exist.',
$templateName
))->line($templateArg->getStartLine())->identifier('twig.templateNotFound')->build();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I dunno if the error strategy need to depends on the PHPStan Level.

For instance, maybe existingTemplate|nonExistingTemplate should only be reported at level 7 or more.
In the same way unions/maybe are reported on level 7 for classic rules.

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

Successfully merging this pull request may close these issues.

verify that the file in render actually exists
3 participants