Skip to content

PHPLIB-1128: Move generate_index_name() to private method within IndexInput #1092

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

Merged
merged 2 commits into from
Jun 1, 2023

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented May 31, 2023

Fix PHPLIB-1128

generate_index_name() was originally made a utility function in bc65629 because it was shared by both the CreateIndexes (via IndexInput) and Count operations. It was later removed from Count in PHPLIB-285.

@GromNaN GromNaN requested review from jmikola and alcaeus May 31, 2023 22:02
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

Please check the two Psalm errors in functions.php. I'm not sure why those were caught in the PR but not locally when you regenerated the baseline.

{
return [
['x_1', ['x' => 1]],
['x_1', (object) ['x' => 1]],
['x_1', new BSONDocument(['x' => 1])],
['x_1', Document::fromPHP(['x' => 1])],
Copy link
Member

Choose a reason for hiding this comment

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

I noted that the original tests for the function were more comprehensive (the four possible type casts are applied to each index specification), but I don't think there's any significant benefit to doing that. Testing a basic index spec with the non-array types seems sufficient.

@GromNaN
Copy link
Member Author

GromNaN commented Jun 1, 2023

I'm fighting with psalm. baseline is not the same for my local PHP 8.2 and the CI PHP 7.4

Error: src/functions.php:86:12: MixedInferredReturnType: Could not verify return type 'array<array-key, mixed>|object' for MongoDB\apply_type_map_to_document (see https://psalm.dev/047)
Error: src/functions.php:95:12: MixedReturnStatement: Could not infer a return type (see https://psalm.dev/138)
Error: Process completed with exit code 2.

I decided to update the PHP version used in the CI in order to be able to generate the baseline from a Mac M1. Otherwise I cannot install the mongodb extension on PHP 7.4, I haven't figured out how to use a polyfill either.

Edit: I managed to manually edit psalm-pipeline.xml to pass the job. New ticket to update PHP version in CI: PHPLIB-1158

@GromNaN GromNaN merged commit bf36331 into mongodb:master Jun 1, 2023
@GromNaN GromNaN deleted the PHPLIB-1128 branch June 1, 2023 18:52
spantaleev added a commit to devture/mongo-php-adapter that referenced this pull request Jul 11, 2023
This function has been relocated to a private method in mongodb/mongo-php-library=1.16.0.
See:
- mongodb/mongo-php-library@bf36331
- mongodb/mongo-php-library#1092

We're copying it inline in our code here for compatibility with 1.16.0.
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.

2 participants