PHPLIB-1057: Deprecate method IndexInfo::isGeoHaystack() #1085
Conversation
| @@ -4,6 +4,9 @@ MongoDB\\Model\\IndexInfo::isGeoHaystack() | |||
|
|
|||
| .. versionadded:: 1.4 | |||
|
|
|||
| .. deprecated:: 1.16 | |||
| MongoDB 5.0 and later no longer supports geoHaystack indexes. | |||
There was a problem hiding this comment.
The doc was updated in #1085 (comment) to mention the feature deprecation. I moved this note to a deprecation block.
| */ | ||
| public function isGeoHaystack() | ||
| { | ||
| trigger_error('MongoDB 5.0 removes support for "geoHaystack" indexes, the method "IndexInfo::isGeoHaystack()" will be removed in a future release', E_USER_DEPRECATED); |
There was a problem hiding this comment.
I was initially going to point out the phpcs errors below and suggest running phpcbf to add imports for trigger_error and E_USER_DEPRECATED.
Thinking about this some more, I don't think we should actually trigger an error here. So long as the driver may be connected to a pre-5.0 server, it's possible for such an index to exist and there's no reason this method should interrupt the user (especially since it's just an "isser" and not something that would enable them to create such an index).
In the other places where we do this, options are unsupported or deprecated in all of our supported server versions but still stuck in our public API.
If PHPC (via the PHPLIB dependnecy) required a 5.0+ server we could certainly add this but I'm not convinced there's much value in doing so. Instead, I'd suggest a separate JIRA ticket with fixVersion 2.0 to just remind us to back and delete this method from the public API. And consider adding a @todo to the doc block like we have in some other classes:
@todo Remove this in 2.0 (see: PHPLIB-XXXX)
There was a problem hiding this comment.
The @deprecated annotation should be enough to trigger the cleanup in 2.0
trigger_deprecated is used for autoIndexId option in MongoDB\Operation\CreateCollection. IMO this is the same situation: the lib triggers a deprecation notice for a feature that is supported on old server version only. This is a trigger for developers that they must update their code to upgrade MongoDB server.
mongo-php-library/src/Operation/CreateCollection.php
Lines 240 to 242 in f75d3a8
Starting in MongoDB 4.0, you cannot set the option autoIndexId to false when creating collections in databases other than the local database.
There was a problem hiding this comment.
The @deprecated annotation should be enough to trigger the cleanup in 2.0
Makes sense.
trigger_deprecatedis used forautoIndexIdoption inMongoDB\Operation\CreateCollection
autoIndexId is an interesting case, as it looks like it's still perfectly usable with non-replicated collections (i.e. local database). I just dug through the old tickets and it looks like my outstanding question in DRIVERS-473, where I raised concerns about that, was never answered. I just opened PHPLIB-1159 to revisit that discussion.
I propose leaving this PR open until we can discuss PHPLIB-1159 as a team. If we're in agreement that leaving trigger_error() on autoIndexId makes sense, then I won't argue with adding it here.
There was a problem hiding this comment.
We decided to keep the deprecation trigger.
| @@ -60,7 +60,9 @@ public function testIsGeoHaystack(): void | |||
| $index = $result->current(); | |||
|
|
|||
| $this->assertEquals($indexName, $index->getName()); | |||
| $this->assertTrue($index->isGeoHaystack()); | |||
| $this->assertDeprecated(function () use ($index): void { | |||
There was a problem hiding this comment.
I forgot this method existed. It dates back to 4b4a7d9#diff-abf1fa77a19f720052b75bc802df0260157b9064d8c3c12a80794f01e9acc065R136.
There was a problem hiding this comment.
I was happy to find it, otherwise I the tests failed.
Symfony has trigger_deprecation and expectDeprecation for tests: this works with symfony/phpunit-bridge that is already used on this library.
MongoDB 5.0 removes support for "geoHaystack" indexes, the method "IndexInfo::isGeoHaystack()" will be removed in a future release
Fix PHPLIB-1057
MongoDB 5.0 removes support for "geoHaystack" indexes, the method "IndexInfo::isGeoHaystack()" will be removed in a future release.