Skip to content

Fix #197 Reset form type detection for each file and class - #198

Open
geelweb wants to merge 2 commits into
php-translation:masterfrom
geelweb:issue-197
Open

geelweb wants to merge 2 commits into
php-translation:masterfrom
geelweb:issue-197

Conversation

@geelweb

@geelweb geelweb commented Oct 29, 2026 •

Copy link
Copy Markdown

Fixes #197

FormTrait::$isFormType was never reset once a form type had been found. As PHPFileExtractor reuses the same visitor instances for every file, all the files (and classes) visited after a form type were considered as form types too. The form visitors then analysed arrays of plain classes, extracting false positive keys and reporting errors on them.

Reset the flag in init(), called before each file, and when entering a new class.

Also fix BaseVisitor::addError() crashing with "Call to a member function getAttribute() on int" when the node is a scalar: scalar nodes have a "value" property holding the raw value, not a Node.

@bocharsky-bw bocharsky-bw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey, thanks for fixing it! We probably have a regression with anonymous classes here. new class {...} in php parser also has Class_ type. Maybe we should skip $node->isAnonymous(), or you have a better idea?

Also, I approved the CI and see some tests are failing, could you take a look? Not sure if it's related to this PR, but if you could fix them - it would help a lot

…lasses

Resetting the detection on every Class_ node broke form types containing
an anonymous class (e.g. a data transformer): the fields declared after
it were no longer extracted. Track the enclosing classes in a stack keyed
on their end position instead, as the visitors define their own leaveNode().

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@geelweb

geelweb commented Oct 30, 2026

Copy link
Copy Markdown
Author

Hey, thanks for fixing it! We probably have a regression with anonymous classes here. new class {...} in php parser also has Class_ type. Maybe we should skip $node->isAnonymous(), or you have a better idea?

You're right, I ask Claude to check and it confirm the regression : the patch resets isFormType on every Class_ and never restores it when the class ends. After the anonymous class, the rest of buildForm() counts as "not a form type". The opposite also happens: an anonymous form type inside a plain class switches the flag on for the rest of that class.

He suggest me to keep a stack of classes, keyed on where each one ends in the file. I added a commit for that.

Also, I approved the CI and see some tests are failing, could you take a look? Not sure if it's related to this PR, but if you could fix them - it would help a lot

my bad.... I forgot to commit tests... I added them

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.

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

2 participants