search

doctrine / DoctrineModule

Doctrine Module for Laminas
http://www.doctrine-project.org/projects/doctrine-module.html
MIT License
398 stars 270 forks source link

Fix setOptions typehints. #805

Closed demiankatz closed 1 year ago

demiankatz commented 1 year ago

The argument 1 typehint on setOption conflicts with the Laminas\Form\ElementInterface -- see: https://github.com/laminas/laminas-form/blob/3.11.x/src/ElementInterface.php#L36

This problem was detected by Psalm when I began work on adding DoctrineModule v6 support to DoctrineORMModule.

This string typehint was introduced in laminas-form v3, so the "mixed" typehint probably worked in combination with v2 and earlier. Since this module depends on laminas-form v3.4.1 or later, there should be no problem with making this adjustment.

greg0ire commented 1 year ago

:thinking: where is the conflict? Both methods seem compatible to me… a method accepting mixed also accepts string, by definition.

It also seems compatible to PHP itself, by the way: https://3v4l.org/Q2FJN… or even to Psalm's playground: https://psalm.dev/r/75cc3938a8

greg0ire commented 1 year ago

I think the real issue is that this job is running on PHP 7.4, while mixed was introduced in php 8.0. This means it interprets mixed as an object type, and since string is not an object, you have a conflict.

demiankatz commented 1 year ago

Interesting, I've gotten so used to PHP being inflexible about types that I didn't realize this could possibly be legal.

That being said, under what circumstance would it make sense to use something other than a string as an option key?

If there is a strong reason, then maybe this needs to be addressed by changing the Psalm configuration in DoctrineORMModule.

greg0ire commented 1 year ago

To be clear, I'm not saying we should not merge your change, but let's first figure out what is so wrong with the Psalm job in DoctrineORMModule that it manages to install a version of the module incompatible with PHP 7.4

greg0ire commented 1 year ago

Ah actually, It's running on php 8.2, but the target version is php 7.4 :thinking:

greg0ire commented 1 year ago

Setting the target php version in the config file, like we do on the ORM fixes the issue: https://github.com/doctrine/orm/blob/2.15.x/psalm.xml#L4

Here is the reason behind specifying it: https://github.com/doctrine/orm/pull/9314#issue-1091878096

demiankatz commented 1 year ago

Thanks, @greg0ire! Would you like me to open a PR to raise the version to match in DoctrineORMModule? If so, I can do that tomorrow morning!

greg0ire commented 1 year ago

Yes please, I think it makes sense to avoid future similar weird surprises.