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

Improve type inference for plugin manager #56

Merged
merged 4 commits into from
Jun 13, 2022

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Jun 13, 2022

Q A
QA yes

Description

If laminas/laminas-servicemanager#137 gets merged, this is what I had in mind for inputfilter, filter, validator and view … and others??

@Ocramius
Copy link
Member

https://github.com/laminas/laminas-servicemanager/releases/tag/3.12.0

@gsteel gsteel force-pushed the qa/plugin-manager-types branch from 4f0f815 to c3d0f02 Compare June 13, 2022 16:29
@@ -87,16 +87,16 @@
},
{
"name": "laminas/laminas-servicemanager",
"version": "3.11.2",
"version": "3.12.0",
Copy link
Member

Choose a reason for hiding this comment

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

We should bump in composer.json

Copy link
Member Author

Choose a reason for hiding this comment

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

Done… I figured the lock would cover it. Is it generally a good idea to bump minimums when adding psalm types not present in older versions?

Copy link
Member

Choose a reason for hiding this comment

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

I think bumping is always a good idea, when it brings some goodies with it :D

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Thanks @gsteel!

@Ocramius Ocramius merged commit a6e8bc7 into laminas:2.17.x Jun 13, 2022
@Ocramius Ocramius self-assigned this Jun 13, 2022
@gsteel gsteel deleted the qa/plugin-manager-types branch June 13, 2022 16:46
@FabianKoestring
Copy link

After this change we got a lot of PHPStan errors.

Return type (array) of method Application\Form\Fieldset\File::getInputFilterSpecification() should be compatible with return type (*NEVER*) of method
         Laminas\InputFilter\InputFilterProviderInterface::getInputFilterSpecification()

Application\Form\Fieldset\File::getInputFilterSpecification() looks like this.

public function getInputFilterSpecification(): array
{
    return [...];
}

@Ocramius
Copy link
Member

@FabianKoestring can you please make a reproducer snippet on https://phpstan.org/try ?

@Ocramius
Copy link
Member

Also, please do so in a new issue :)

@FabianKoestring
Copy link

@FabianKoestring can you please make a reproducer snippet on https://phpstan.org/try ?

I would do that but I don't know how to set dependencies (to laminas/laminas-inpufilter) on https://phpstan.org/try. The problem occurs from version 2.16.0 on.

@Ocramius
Copy link
Member

@FabianKoestring copy in all symbols that affect your issue, and reduce the example to the minimum size to show the issue.

@FabianKoestring
Copy link

@FabianKoestring copy in all symbols that affect your issue, and reduce the example to the minimum size to show the issue.

👍 Look at error on line 62. https://phpstan.org/r/91e0e318-0ada-4777-9e50-983a0d7678a1

Return type (CollectionSpecification|InputFilterSpecification) of method HelloWorld::getInputFilterSpecification() should be compatible with return type (NEVER) of method InputFilterProviderInterface::getInputFilterSpecification()

@Ocramius
Copy link
Member

Simplifying your example further, it looks like phpstan doesn't understand this syntax:

array{
     type?: class-string<InputFilterInterface>|string,
}&array<array-key, InputSpecification>

See https://phpstan.org/r/6e758234-f4ef-4ff6-8519-919f3c9297e4

I would report an issue in upstream phpstan/phpstan-src

@FabianKoestring
Copy link

@froschdesign
Copy link
Member

@gsteel
Please check phpstan/phpstan#7475 (comment)

@gsteel
Copy link
Member Author

gsteel commented Jun 15, 2022

The problem with the InputFilterSpecification type is that is seems valid to me. From what I can tell, the factory accepts [?'type' => 'FQCN or alias'] and also the array will contain a list of either Inputs or InputFilters or specification arrays. The keys of the list can be strings or integers - string keys are used as the input name (This is the assumed behaviour) and integer keys are replaced with the 'name' from the input spec (value).

At the expense of allowing integer keys, the type could be array{type?: whatever}&array<string, InputSpec|InstanceType>

It currently seems to me that the intersection type is the right thing but I'd gladly learn of a better/more precise way to represent this…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants