[SF][Form] Form type detection leaks to the following files, crashes on 'choices' => <int> outside of forms #197

Description

Since FormTrait was introduced (#184 / #185), once a form type has been visited, every file visited afterwards is considered as a form type. The form visitors then analyse arrays of plain classes (repositories, services...), which leads to:

1. a fatal error when such a class contains an array item 'choices' => <int>:

   Error: Call to a member function getAttribute() on int
     at src/Visitor/BaseVisitor.php:58
     Translation\Extractor\Visitor\BaseVisitor->addError() at src/Visitor/Php/Symfony/FormTypeChoices.php:114
     Translation\Extractor\Visitor\Php\Symfony\FormTypeChoices->enterNode() at vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:114
     ...
     Translation\Extractor\FileExtractor\PHPFileExtractor->getSourceLocations() at src/Extractor.php:55
   

2. false positives: keys extracted from 'label' => '...', 'placeholder' => '...', etc. array items of non form classes, and errors like "Form label is not a scalar string" reported on them.

Version: 2.4.2

How to reproduce

Two files, visited in this order (the Finder sorts them by name):

<?php
// fixtures/AFormType.php
namespace Repro;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormBuilderInterface;

class AFormType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder->add('kind', null, ['choices' => ['Kind one' => 1]]);
    }
}

<?php
// fixtures/BRepository.php
namespace Repro;

class BRepository
{
    public function counts(): array
    {
        return ['groups' => 0, 'choices' => 0];
    }
}

use Symfony\Component\Finder\Finder;
use Translation\Extractor\Extractor;
use Translation\Extractor\FileExtractor\PHPFileExtractor;
use Translation\Extractor\Visitor\Php\Symfony\FormTypeChoices;

$fileExtractor = new PHPFileExtractor();
$fileExtractor->addVisitor(new FormTypeChoices());
$extractor = new Extractor();
$extractor->addFileExtractor($fileExtractor);

$extractor->extract((new Finder())->files()->in(__DIR__.'/fixtures')->sortByName());
// Error: Call to a member function getAttribute() on int

Expected: Kind one is extracted, BRepository is ignored.

Cause

There are 2 issues:

1. FormTrait::$isFormType is set to true when a form type class is found, but it is never reset. As the same visitor instances are reused by PHPFileExtractor for every file, the flag leaks to all the following files (and to the following classes of the same file).

2. BaseVisitor::addError() uses $node->value->getAttribute() as soon as the node has a value property. Scalar nodes (Scalar\Int_, Scalar\String_, Scalar\Float_...) also have a value property, holding the raw PHP value, hence the fatal error.

Proposed fix

--- a/src/Visitor/Php/Symfony/FormTrait.php
+++ b/src/Visitor/Php/Symfony/FormTrait.php
@@ -16,6 +16,8 @@ use PhpParser\Node\Stmt\Class_;
 use PhpParser\NodeTraverser;
 use PhpParser\NodeVisitor\NameResolver;
 use PhpParser\ParserFactory;
+use Symfony\Component\Finder\SplFileInfo;
+use Translation\Extractor\Model\SourceCollection;
 
 trait FormTrait
 {
@@ -23,12 +25,26 @@ trait FormTrait
     private array $classMap = [];
     private array $symfonyInterfaces = ['FormTypeInterface', 'FormTypeExtensionInterface'];
 
+    /**
+     * The visitor is reused for every file: the form type detection of the
+     * previous file must not leak to the next one.
+     */
+    public function init(SourceCollection $collection, SplFileInfo $file): void
+    {
+        $this->isFormType = false;
+
+        parent::init($collection, $file);
+    }
+
     protected function isFormType(Node $node): bool
     {
         if (!$node instanceof Class_) {
             return $this->isFormType;
         }
 
+        // Each class must be checked on its own
+        $this->isFormType = false;
+
         $allInterfaces = $this->getAllInterfacesFromNode($node);
 
         foreach ($allInterfaces as $interface) {
--- a/src/Visitor/BaseVisitor.php
+++ b/src/Visitor/BaseVisitor.php
@@ -54,7 +54,8 @@ abstract class BaseVisitor implements Visitor
         $docComment = $node->getDocComment();
         $file = $this->getAbsoluteFilePath();
 
-        if (property_exists($node, 'value')) {
+        // Scalar nodes also have a "value" property, holding the raw value
+        if (property_exists($node, 'value') && $node->value instanceof Node) {
             $line = $node->value->getAttribute('startLine');
         } else {
             $line = $node->getAttribute('startLine');

init() is called by PHPFileExtractor before each file, so overriding it in the trait fixes the 8 visitors using FormTrait at once (none of them overrides init()).

Note for users: projects may currently rely on the false positives, i.e. keys only extracted because a non form class was wrongly visited as a form type. Those keys will no longer be extracted with this fix.

I can open a PR with this fix and a test if you agree with the approach.