-
-
Notifications
You must be signed in to change notification settings - Fork 506
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
Conversation
There was a problem hiding this 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.
a013af3
to
b9a5745
Compare
Sharding failures seem unrelated: https://github.com/doctrine/mongodb-odm/actions/runs/8914210744/job/24481504241?pr=2630 |
|
||
#[Index(keys: ['topic' => 'asc'])] | ||
#[ODM\Index(keys: ['topic' => 'asc'])] | ||
#[ODM\SearchIndex(dynamic: true)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
lib/Doctrine/ODM/MongoDB/Tools/Console/Command/Schema/AbstractCommand.php
Show resolved
Hide resolved
lib/Doctrine/ODM/MongoDB/Tools/Console/Command/Schema/CreateCommand.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/ODM/MongoDB/Tools/Console/Command/Schema/DropCommand.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/ODM/MongoDB/Tools/Console/Command/Schema/UpdateCommand.php
Outdated
Show resolved
Hide resolved
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):
UnitOfWork is also entirely unrelated to this PR, so I'm not sure what to make of that. |
This could be related to using an outdated psalm version - I was able to reproduce it locally with psalm 5.24.0. |
There was a problem hiding this 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.
tests/Doctrine/ODM/MongoDB/Tests/Mapping/AbstractMappingDriverTestCase.php
Show resolved
Hide resolved
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.
There was a problem hiding this 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.
Summary