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

Update call maps for MongoDB extension #8432

Merged
merged 3 commits into from Sep 8, 2022

Conversation

alcaeus
Copy link
Contributor

@alcaeus alcaeus commented Aug 24, 2022

While adding psalm checks to the MongoDB driver, I noticed that a number of method signatures in the call map were either missing or out-of-date. This PR updates them to what we'll ship in 1.15. They have been automatically generated from the definitions in the driver and made to match the format required by Psalm.

@orklah orklah added the release:feature The PR will be included in 'Features' section of the release notes label Aug 24, 2022
@orklah
Copy link
Collaborator

orklah commented Aug 24, 2022

Thanks! Can you look at the mismatch reported in Unit Tests 4?

@alcaeus
Copy link
Contributor Author

alcaeus commented Aug 25, 2022

Ah, there were some more (lowercased) entries in a completely separate portion of the file. These were now duplicated, but since they were originally lowercased they were treated as different. I removed the obsolete definitions.

@alcaeus
Copy link
Contributor Author

alcaeus commented Aug 25, 2022

I noticed that some of the interface declarations aren't correct. Please hold off on merging while I figure out where the script failed (probably has to do with tentative return types).

@alcaeus
Copy link
Contributor Author

alcaeus commented Aug 25, 2022

I noticed that some of the interface declarations aren't correct. Please hold off on merging while I figure out where the script failed (probably has to do with tentative return types).

That's fixed now. I didn't consider tentative return types for interfaces.

Copy link
Collaborator

@orklah orklah left a comment

Choose a reason for hiding this comment

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

Can you sort declaration in alphabetical order? It seems to be the previous order and it would help reviewing. Please see a few notes also

'MongoDB\BSON\Decimal128::__construct' => ['void', 'value='=>'string'],
'MongoDB\BSON\Binary::__toString' => ['string'],
'MongoDB\BSON\Binary::serialize' => ['string'],
'MongoDB\BSON\Binary::unserialize' => ['void', 'serialized' => ''],
Copy link
Collaborator

Choose a reason for hiding this comment

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

serialized should have a type here, either mixed or more narrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks. Added the correct string type for all unserialize methods.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see a new commit on the branch? Forgot to push?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whopsie, yes. I pushed the commit now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I have bad news then, I still don't see anything

Copy link
Collaborator

Choose a reason for hiding this comment

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

nvm, I was mistaken apparently

'MongoDB\BSON\BinaryInterface::__toString' => ['string'],
'MongoDB\BSON\DBPointer::__toString' => ['string'],
'MongoDB\BSON\DBPointer::serialize' => ['string'],
'MongoDB\BSON\DBPointer::unserialize' => ['void', 'serialized' => ''],
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

dictionaries/CallMap.php Show resolved Hide resolved
@alcaeus alcaeus requested a review from orklah August 26, 2022 08:06
@orklah
Copy link
Collaborator

orklah commented Sep 8, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants