laminas / laminas-form

Validate and display simple and complex forms, casting forms to business objects and vice versa
https://docs.laminas.dev/laminas-form/
BSD 3-Clause "New" or "Revised" License
80 stars 52 forks source link

Introduce Form Template to improve inference for processed payloads #248

Closed gsteel closed 11 months ago

gsteel commented 11 months ago
Q A
New Feature yes
RFC yes

Description

Introduce Form Template to improve inference for processed payloads…

This patch should allow users to provide templates for form classes with full inference of the resulting valid payload.

Added an interface method, which we could probably revert considering that Form::getData() has decent inference if you explicitly pass a flag, due to the conditional return on the interface.

There are some weird new issues around inheritance and fluent return types which I'm leaving here for now because I have no idea how to fix them, or why they were introduced by these changes. Maybe they are fixed in 5.16, so I'll update there…

Closes #238

gsteel commented 11 months ago

Psalm 5.16 doesn't fix the new LessSpecificReturnStatement problems. Maybe someone can take a look and see if they have any idea what's going on there.

gsteel commented 11 months ago

Sussed it, All/most of the fluid return types in Form are annotated with @return $this, when inherited, psalm infers the return type as FormSubclass<TFilteredValues> and $this is therefore LessSpecific. Returning self or $this<TFilteredValues>, fixes the issues. I'll update the patch with the latter, because it is more precise

Ok, I don't actually think that psalm likes $this<TFilteredValues> at all. I found that either self or self<TFilteredValues> solved the issues in very specific places, but wholesale replacement of $this yielded many more errors.

I've updated the Static Analysis test case to illustrate template preservation using fluid return types, but not for all possible methods.

Also, I thought that adding an interface method might be a BC break? Happy to revert that if is.

gsteel commented 11 months ago

@Slamdunk Can I baseline the remaining issues please 🙏 I feel like the issues are concerned with the deep inheritance hierarchy where everything extends from Element. You'll see that the issues are not applied to a SomeUserForm when extending from Form but to ancestors such as Element and Fieldset.

I'd like to try and keep this patch alive because the additional inference provided to consumers would be really useful for a project I'm working on 😁

Slamdunk commented 11 months ago

Can I baseline the remaining issues please 🙏

As long as you give me a reasonable guarantee that users' PSalm runs won't explode with unresolvable errors after this patch, I'm totally ok with this :+1:

gsteel commented 11 months ago

To verify that these psalm issues don't crop up for inheritors of Form and Fieldset etc, I've fixed a load of issues with test assets to remove them from baseline completely.

Thanks @Slamdunk

Ocramius commented 11 months ago

Worth releasing: triggered a release