Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type refinement for options in form types #400

Open
stof opened this issue Jun 27, 2024 · 2 comments
Open

Type refinement for options in form types #400

stof opened this issue Jun 27, 2024 · 2 comments

Comments

@stof
Copy link
Contributor

stof commented Jun 27, 2024

the $options argument of the various methods in FormTypeInterface and FormTypeExtensionInterface are typed as array<string, mixed> for now, because there is no other way to type them in phpdoc. However, the actual type is a configurable array shape, depending on the configuration of the OptionsResolver.
Having a more precise type would make it easier to write form types when using the level 9 of phpstan (where mixed requires explicit type checking to use them) and would allow more precise analysis at lower levels (as phpstan would report invalid usages of values taken from options instead of allowing anything because they are mixed)

Calls done on the OptionsResolver in the configureOptions method should be analyzed to build the array shape:

  • setDefined marks a key (or multiple ones) as known (but the key might be missing)
  • setRequired will mark a key (or multiple ones) as known and the key is guaranteed to be present
  • setDefault will mark the key as known and the key is guaranteed to be present
  • setDefaults is equivalent to calling setDefault for each key/value pair of the argument
  • setAllowedTypes defines the allowed types for that option. When multiple values are passed in the array, this allows a union of those types. Supported types are built-in type names, object types (class names or interface names) or generic array types (defines with a [] suffix after a supported type). Passing a string as second argument is equivalent to passing an array containing that string as single value.
  • addAllowedTypes allows to expand the list of allowed types by adding more types in the union (if no allowed types have been configured yet for that option, either through setAllowedTypes or addAllowedTypes, it is equivalent to setAllowedTypes)
  • setAllowedValues allows to define the list of allowed values for a property (passing anything else than an array as second argument is treated like passing a single-value array):
    • either a Closure (not other types of callables) taking the value as argument and returning a boolean to decide whether it is allowed (which would probably mean that phpstan has to skip the refinement based on allowed values)
    • or a value which will be compared with ===. This might lead to refining the option to a union of string literals for instance.
  • setDefault (and setDefaults) support defining a nested option by using a Closure with a first argument using Symfony\Component\OptionsResolver\OptionsResolver (exactly) as argument type. the type of the value will depend on the array shape for the OptionsResolver configured in that Closure:
    • if setPrototype(true) was called in the configuration, the nested option contains an array of values matching the array shape
    • otherwise, the nested option will match the array shape

For options that are defined without defining allowed types or allowed values (or defining callback-based validation of allowed values that cannot be analyzed), the array shape should used mixed as the type for them.

A basic version of the analysis could report only options defined by a form type for itself. However, this might return false positive errors for some valid code relying on parent options. A full analysis would have to consider the type hierarchy:

  • in a type extension, merge the array shape configuration with the shape of the extended types (based on class names returned in getExtendedTypes). If an extension extends multiple types, this would lead to a union type of a shape for each type
  • in a form type, merge the array shape configuration with the shape or the parent defined in getParent() (if not null)

Note: the array shape should be an unsealed one (if PHPStan ever implements the distinction between sealed and unsealed shapes that Psalm implements) because all form types of the type hierarchy receive the full array of options (so when calling methods on a parent type, it is likely to contain some extra options known only by the child type or a type extension).

@kevinpapst
Copy link

As I am currently struggling with that exact topic, here is a short & simple reproducer:

final class SomeDateHandlerForm extends AbstractType {
  public function buildForm(FormBuilderInterface $builder, array $options): void {
        $date = $options['year'] . '-01-01';
        // ... 
    }

    public function configureOptions(OptionsResolver $resolver): void {
        $resolver->setDefaults(['year' => date('Y')]);
        $resolver->setAllowedTypes('year', ['string']);
    }
}

This will report Binary operation "." between mixed and '-01-01' results in an error. even though the OptionsResolver makes sure that only a string is given in $options['year']. Currently I have to either silence the line or write type checking logic, which is not necessary due to the validation inside OptionsResolver.

@ruudk
Copy link
Contributor

ruudk commented Sep 18, 2024

Worked on a fixture that can be used to build this feature. It uses the unsealed array shape syntax (that's not yet supported by PHPStan itself but is on the PHPDoc parser).

What would be the best PHPStan extension point to analyze all the calls in configureOptions before analyzing buildForm? Should it be something similar to phpstan-doctrine's QueryBuilder trick?
https://github.com/phpstan/phpstan-doctrine/blob/2.0.x/src/Type/Doctrine/QueryBuilder/CreateQueryBuilderDynamicReturnTypeExtension.php

Ideally, you only want this to trigger when inside configureOptions on an AbstractType class.

class DataClassType extends AbstractType
{
	public function buildForm(FormBuilderInterface $builder, array $options): void
	{
		assertType('array{firstName: string, lastName?: null|string, roles: array, expiresAt: DateTimeInterface|null, ...<string, mixed>}', $options);
	}

	public function configureOptions(OptionsResolver $resolver): void
	{
		$resolver->setRequired([
			'firstName',
		]);

		$resolver->setAllowedTypes('firstName', 'string');
		$resolver->setAllowedTypes('lastName', ['null', 'string']);
		$resolver->setAllowedTypes('roles', 'array');
		$resolver->setAllowedTypes('expiresAt', [DateTimeInterface::class, 'null']);
	}
}

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

No branches or pull requests

3 participants