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

Support search indexes in mappings and SchemaManager #2630

Merged
merged 15 commits into from
May 22, 2024

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Apr 24, 2024

Q A
Type feature
BC Break no
Fixed issues PHPORM-38

Summary

@alcaeus alcaeus self-requested a review April 25, 2024 13:33
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Initial review looks good. There's a number of PHPStan issues due to not properly specifying all array key/value pairs. I'll let you decide whether it's worth specifying them all or whether you'd rather add the errors to the baseline.

doctrine-mongo-mapping.xsd Outdated Show resolved Hide resolved
lib/Doctrine/ODM/MongoDB/SchemaManager.php Show resolved Hide resolved
@jmikola jmikola marked this pull request as ready for review May 1, 2024 17:09
@jmikola jmikola force-pushed the 2.8-phporm-38 branch 5 times, most recently from a013af3 to b9a5745 Compare May 1, 2024 19:52
@jmikola
Copy link
Member Author

jmikola commented May 1, 2024


#[Index(keys: ['topic' => 'asc'])]
#[ODM\Index(keys: ['topic' => 'asc'])]
#[ODM\SearchIndex(dynamic: true)]
Copy link
Member

Choose a reason for hiding this comment

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

Could you use all the fields in the attribute to make the test more exhaustive?

Copy link
Member Author

Choose a reason for hiding this comment

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

The mapping tests are exhaustive in their use of all options. This model is only used for the SchemeManager tests, in which we assert that something is passed along to the mocked MongoDB\Collection methods. That is based on prior art for the standard index tests.

@jmikola
Copy link
Member Author

jmikola commented May 6, 2024

Psalm is failing due to two unused baseline entries, but I'm unable to reproduce that locally with PHP 8.2 (same as is used in CI):

Error: lib/Doctrine/ODM/MongoDB/UnitOfWork.php:0:0: UnusedBaselineEntry: Baseline for issue "TypeDoesNotContainType" has 1 extra entry. (see https://psalm.dev/316)
Error: tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php:0:0: UnusedBaselineEntry: Baseline for issue "TypeDoesNotContainType" has 1 extra entry. (see https://psalm.dev/316)

UnitOfWork is also entirely unrelated to this PR, so I'm not sure what to make of that.

@alcaeus
Copy link
Member

alcaeus commented May 7, 2024

Psalm is failing due to two unused baseline entries, but I'm unable to reproduce that locally with PHP 8.2 (same as is used in CI):

This could be related to using an outdated psalm version - I was able to reproduce it locally with psalm 5.24.0.

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

LGTM with psalm issues fixed.

jmikola added 13 commits May 13, 2024 11:48
Adds default implementations for "process" methods in AbstractCommand, which throw BadMethodCallException.

Renames internal methods in ShardCommand to no longer override "index" base methods, since sharding methods in SchemaManager do much more than process indexes.
The fields struct is recursive, which is not supported by phpstan. The analyzers struct may be technically possible to model, but the complexity isn't worth the effort.
Currently, commands can either process all definitions (default behavior) or specify individual definitions. This allows the commands to rely on default behavior (e.g. $createOrder) but omit processing of search indexes, which may be more stringent requirements.

Note: this is similar to the disable-validators option that already existed in UpdateCommand; however, doctrine#2634 suggests renaming that if additional "skip" options are introduced.
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Gave the docs a read and they look good to me. Since we can't easily test docs generation, I'd suggest we merge this as is and fix any RST issues after the fact.

@jmikola jmikola merged commit fed919f into doctrine:2.8.x May 22, 2024
16 of 17 checks passed
@jmikola jmikola deleted the 2.8-phporm-38 branch May 22, 2024 12:53
@jmikola jmikola added this to the 2.8.0 milestone May 22, 2024
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.

None yet

4 participants