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

Fix setOptions typehints. #805

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

demiankatz
Copy link
Contributor

@demiankatz demiankatz commented May 11, 2023

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
Copy link
Member

greg0ire commented Jun 7, 2023

🤔 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
Copy link
Member

greg0ire commented Jun 7, 2023

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
Copy link
Contributor Author

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
Copy link
Member

greg0ire commented Jun 7, 2023

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
Copy link
Member

greg0ire commented Jun 7, 2023

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

@greg0ire
Copy link
Member

greg0ire commented Jun 7, 2023

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: doctrine/orm#9314 (comment)

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Although this looks like a breaking change for callers, since the parent method is called, $key has to be a string every time this is called.

@greg0ire greg0ire requested review from TomHAnderson and driehle June 7, 2023 21:06
@demiankatz
Copy link
Contributor Author

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
Copy link
Member

greg0ire commented Jun 7, 2023

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

@TomHAnderson TomHAnderson merged commit a63fb62 into doctrine:6.0.x Jun 7, 2023
@demiankatz demiankatz deleted the fix-setoptions-typehints branch June 8, 2023 10:27
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

Successfully merging this pull request may close these issues.

3 participants