Fix #184: Check FormTypeInterface instead of class name suffix

[gchehami]

Description

This PR fixes the form detection logic to check if a class implements FormTypeInterface instead of checking if the class name ends with "Type" only.

Fixes #184

Changes

• Modified FormTrait.php to check for FormTypeInterface implementation
• Now supports forms with any naming convention (e.g., MyForm, MyFormType, CustomFormType)

Motivation

There are no rules that impose naming forms with the word "Type" at the end of classnames. Developers may name their forms with different suffixes like "Form" or any other convention. The correct way is to check if the class implements the FormTypeInterface.

Testing

• Existing tests should pass
• Forms not ending with "Type" are now correctly detected

[bocharsky-bw]

Yeah, you're right, checking for the interface is much better... I would say maybe we should drop the legacy way of checking for Type suffix in favor of your way based on the interface implementation? WDYT?

Also, could you please check the failed SA tools?

[gchehami]

Yeah, you're right, checking for the interface is much better... I would say maybe we should drop the legacy way of checking for Type suffix in favor of your way based on the interface implementation? WDYT?

Also, could you please check the failed SA tools?

I initially considered this approach, but a colleague pointed out that it could be a breaking change for users who have classes ending with "Type" that don't implement the interface.

i check for the failed SA tools

[bocharsky-bw]

I initially considered this approach, but a colleague pointed out that it could be a breaking change for users who have classes ending with "Type" that don't implement the interface.

Hm, but we're talking about forms here, and you always have to implement that interface for your form types, right? If so, I would not consider it as the BC break actually. I can't think of when you have a form type but don't extend that interface. Do you have any use cases for this?

[gchehami]

I initially considered this approach, but a colleague pointed out that it could be a breaking change for users who have classes ending with "Type" that don't implement the interface.

Hm, but we're talking about forms here, and you always have to implement that interface for your form types, right? If so, I would not consider it as the BC break actually. I can't think of when you have a form type but don't extend that interface. Do you have any use cases for this?

No i don't have any use case for this so okay i remove the check

[bocharsky-bw]

Hm, the latest changes caused errors in CI, could you double-check? It should be something minor I suppose

[gchehami]

Hello, so i tried the first solution and it was good if the first class implements the interface but if the class extends another class that implements the interface then the first class was not detected has formType so i improve the code to get the interface of extended classes with récursive call to get all the interfaces and be sure one of them has FormTypeInterface. i Also add some Unit Test to validate. I think i have the good solution now.

[bocharsky-bw]

Good catch with the recursive check, thank you! Could you fix the PHP CS Fixer issues here, please?

[bocharsky-bw]

I also left some comments

[bocharsky-bw]

Hm, extends AbstractType should also work, right? I suppose we should revert it.

I mean, we should check both: extends AbstractType and implements FormTypeInterface

I see you already check for implements FormTypeInterface in other places, so I would prefer to revert back to extends AbstractType in those spots where you changed to interface to make sure it's still works too

[gchehami]

If i extends AbstractType i need to require dev symfony/form or create a AbstractType class that implements FormTypeInterface in test/Resources/Php/Symfony.

[bocharsky-bw]

Hm, I see.. Well, then I'm fine with using the interface. Though it would be cool to have a test that will cover the case with an extended base class like AbstractType to make sure it finds that interface implemented even in the parent base class we extend. Maybe creating a custom AbstractType that implements FormTypeInterface just for test purposes would make sense 👍🏼

[gchehami]

I did a test, where a formType extends a parentType that implements FormTypeInterface, but if you prefer to call it AbstractType i can change the name of the parentType class

The test is : IsFormTypeTest

I made it because i needed to validate my recursive verification

[bocharsky-bw]

Ah ok, I missed that... That's good to have this test to check the recursive feature, good job 👍🏼

[gchehami]

Good catch with the recursive check, thank you! Could you fix the PHP CS Fixer issues here, please?

yes i'll do it :)

[bocharsky-bw]

OK, I think only failed PHP CS Fixer left here :)

[gchehami]

OK, I think only failed PHP CS Fixer left here :)

Normally it's all good 👍

[bocharsky-bw]

Ah, seems PHPstan also has some failures

[gchehami]

Ah, seems PHPstan also has some failures

It seems that the jakzal/phpqa:php8.1-alpine Docker image ships with its own pre-installed version of php-parser, independent from the one installed by composer. PHPStan likely uses its internal php-parser which is probably v4, which generates the error. To avoid the issue, I preferred to ignore the two problematic lines.

I don't have the issue locally

[bocharsky-bw]

Thank you for working on it!