diff --git a/src/Visitor/BaseVisitor.php b/src/Visitor/BaseVisitor.php index 8ad0102..40ca1ad 100644 --- a/src/Visitor/BaseVisitor.php +++ b/src/Visitor/BaseVisitor.php @@ -54,7 +54,8 @@ protected function addError(Node $node, string $errorMessage): void $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'); diff --git a/src/Visitor/Php/Symfony/FormTrait.php b/src/Visitor/Php/Symfony/FormTrait.php index 546c4ce..f5c998f 100644 --- a/src/Visitor/Php/Symfony/FormTrait.php +++ b/src/Visitor/Php/Symfony/FormTrait.php @@ -16,31 +16,60 @@ use PhpParser\NodeTraverser; use PhpParser\NodeVisitor\NameResolver; use PhpParser\ParserFactory; +use Symfony\Component\Finder\SplFileInfo; +use Translation\Extractor\Model\SourceCollection; trait FormTrait { - private bool $isFormType = false; + /** + * Classes being traversed, innermost last: [end position in file, is form type]. + * + * @var list + */ + private array $classStack = []; 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->classStack = []; + + parent::init($collection, $file); + } + + /** + * Must be called for every node, in traversal order. The enclosing classes + * are tracked by position because the visitors define their own leaveNode(). + */ protected function isFormType(Node $node): bool { - if (!$node instanceof Class_) { - return $this->isFormType; + // Leave the classes (named or anonymous) that end before this node + while ($this->classStack && end($this->classStack)[0] < $node->getStartFilePos()) { + array_pop($this->classStack); } - $allInterfaces = $this->getAllInterfacesFromNode($node); + if ($node instanceof Class_) { + $this->classStack[] = [$node->getEndFilePos(), $this->implementsFormInterface($node)]; + } + + return $this->classStack ? end($this->classStack)[1] : false; + } - foreach ($allInterfaces as $interface) { + private function implementsFormInterface(Class_ $node): bool + { + foreach ($this->getAllInterfacesFromNode($node) as $interface) { foreach ($this->symfonyInterfaces as $symfonyInterface) { if ($interface === $symfonyInterface || str_ends_with($interface, '\\'.$symfonyInterface)) { - $this->isFormType = true; - break; + return true; } } } - return $this->isFormType; + return false; } protected function getAllInterfacesFromNode(Class_ $node): array diff --git a/tests/Functional/Visitor/Php/Symfony/IsFormTypeTest.php b/tests/Functional/Visitor/Php/Symfony/IsFormTypeTest.php index 7e8695d..7171db1 100644 --- a/tests/Functional/Visitor/Php/Symfony/IsFormTypeTest.php +++ b/tests/Functional/Visitor/Php/Symfony/IsFormTypeTest.php @@ -41,4 +41,54 @@ public function testNonFormTypeDoesNotExtractTranslations(): void $this->assertCount(0, $collection); } + + /** + * @see https://github.com/php-translation/extractor/issues/197 + */ + public function testFormTypeDetectionDoesNotLeakToNextFile(): void + { + $visitor = new FormTypeLabelExplicit(); + + $collection = $this->getSourceLocations($visitor, Resources\Php\Symfony\ParentType::class); + $this->assertCount(1, $collection); + + $collection = $this->getSourceLocations($visitor, Resources\Php\Symfony\NotAFormType::class); + $this->assertCount(0, $collection); + } + + /** + * @see https://github.com/php-translation/extractor/issues/197 + */ + public function testFormTypeDetectionDoesNotLeakToNextClass(): void + { + $collection = $this->getSourceLocations( + new FormTypeLabelExplicit(), + Resources\Php\Symfony\FormTypeFollowedByClass::class + ); + + $this->assertCount(1, $collection); + $this->assertEquals('form.field.label', $collection->first()->getMessage()); + } + + /** + * The form type detection is restored once an anonymous class ends, in both directions. + */ + public function testAnonymousClasses(): void + { + $collection = $this->getSourceLocations( + new FormTypeLabelExplicit(), + Resources\Php\Symfony\AnonymousClasses::class + ); + + $messages = []; + foreach ($collection as $source) { + $messages[] = $source->getMessage(); + } + + $this->assertEquals([ + 'before.anonymous.class', + 'after.anonymous.class', + 'inside.anonymous.form.type', + ], $messages); + } } diff --git a/tests/Resources/Github/Issue_71.php b/tests/Resources/Github/Issue_71.php index 905e743..8d19f63 100644 --- a/tests/Resources/Github/Issue_71.php +++ b/tests/Resources/Github/Issue_71.php @@ -11,10 +11,10 @@ class TestController extends Controller public function foo() { $foo = [ - ['label' => $this->get('translator')->trans('this.should.be.translated.without.errors')], //ERROR: Form label is not a scalar string + ['label' => $this->get('translator')->trans('this.should.be.translated.without.errors')], [/** @Ignore */'label' => $this->get('translator')->trans('this.should.be.translated.without.errors.and.now.it.works')], ['label_' => $this->get('translator')->trans('this.works.correctly')], - ['label' => $this->shouldBeIgnored()], //ERROR: Form label is not a scalar string + ['label' => $this->shouldBeIgnored()], [/** @Ignore */'label' => $this->shouldBeIgnoredAndGoesWithoutError()], ]; } diff --git a/tests/Resources/Php/Symfony/AnonymousClasses.php b/tests/Resources/Php/Symfony/AnonymousClasses.php new file mode 100644 index 0000000..9e5f8ab --- /dev/null +++ b/tests/Resources/Php/Symfony/AnonymousClasses.php @@ -0,0 +1,50 @@ +add('before', null, [ + 'label' => 'before.anonymous.class' + ]); + $builder->get('before')->addModelTransformer(new class implements DataTransformerInterface { + public function transform($value): mixed + { + return ['label' => 'inside.anonymous.class']; + } + + public function reverseTransform($value): mixed + { + return $value; + } + }); + $builder->add('after', null, [ + 'label' => 'after.anonymous.class' + ]); + } +} + +class ClassWithAnonymousFormType +{ + public function createType(): AbstractType + { + $type = new class extends AbstractType { + public function buildForm(FormBuilderInterface $builder, array $options): void + { + $builder->add('field', null, [ + 'label' => 'inside.anonymous.form.type' + ]); + } + }; + + $notAForm = ['label' => 'after.anonymous.form.type']; + + return $type; + } +} diff --git a/tests/Resources/Php/Symfony/FormTypeFollowedByClass.php b/tests/Resources/Php/Symfony/FormTypeFollowedByClass.php new file mode 100644 index 0000000..218ca3e --- /dev/null +++ b/tests/Resources/Php/Symfony/FormTypeFollowedByClass.php @@ -0,0 +1,26 @@ +add('field', null, [ + 'label' => 'form.field.label' + ]); + } +} + +class ClassFollowingFormType +{ + public function buildForm(FormBuilderInterface $builder, array $options): void + { + $builder->add('field', null, [ + 'label' => 'should.not.be.extracted' + ]); + } +} diff --git a/tests/Smoke/AllExtractorsTest.php b/tests/Smoke/AllExtractorsTest.php index a2b82bc..e4c6dac 100644 --- a/tests/Smoke/AllExtractorsTest.php +++ b/tests/Smoke/AllExtractorsTest.php @@ -94,7 +94,7 @@ public function testNoException(): void * It is okey to increase the error count if you adding more fixtures/code. * We just need to be aware that it changes. */ - $this->assertCount(15, $sc->getErrors(), 'There was an unexpected number of errors. Please investigate.'); + $this->assertCount(13, $sc->getErrors(), 'There was an unexpected number of errors. Please investigate.'); } /**