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 several CallMap function signatures #8217

Merged
merged 4 commits into from Jul 6, 2022

Conversation

othercorey
Copy link
Contributor

Fixed several standard functions.

@othercorey othercorey force-pushed the fix-callmap-funcs branch 2 times, most recently from d7afd82 to 4b77691 Compare July 5, 2022 05:59
@@ -10001,7 +10001,7 @@
'copy' => ['bool', 'from'=>'string', 'to'=>'string', 'context='=>'resource'],
'cos' => ['float', 'num'=>'float'],
'cosh' => ['float', 'num'=>'float'],
'count' => ['int', 'value'=>'Countable|array|SimpleXMLElement|ResourceBundle', 'mode='=>'int'],
'count' => ['int', 'value'=>'mixed', 'mode='=>'int'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this was intentionally more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SimpleXMLElement and ResourceBundle implement Countable so they aren't necessary. Should we just make it Countable|array for all versions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not for historical versions: https://3v4l.org/N8LQ5

Copy link
Collaborator

Choose a reason for hiding this comment

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

Though I'm fine removing SimpleXMLElement from the signature starting from 7.4. Less complexity is always good. Not sure about ResourceBundle though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to check whether it always implemented that interface too. I'll adjust it back up based on that.

Some of these are tricky to follow the implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the changelog ResourceBundle started implementing Countable in 7.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restored the historical signature and moved the change to 8.0 where the TypeError was thrown.

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Jul 6, 2022
@orklah
Copy link
Collaborator

orklah commented Jul 6, 2022

Thanks!

@orklah orklah merged commit e28cd12 into vimeo:4.x Jul 6, 2022
@othercorey othercorey deleted the fix-callmap-funcs branch July 6, 2022 05:27
kkmuffme added a commit to kkmuffme/psalm that referenced this pull request Aug 16, 2022
kkmuffme added a commit to kkmuffme/psalm that referenced this pull request Sep 11, 2022
kkmuffme added a commit to kkmuffme/psalm that referenced this pull request Sep 21, 2022
kkmuffme added a commit to kkmuffme/psalm that referenced this pull request Sep 21, 2022
kkmuffme added a commit to kkmuffme/psalm that referenced this pull request Sep 21, 2022
kkmuffme added a commit to kkmuffme/psalm that referenced this pull request Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants