Skip to content

PHPLIB-1057: Deprecate method IndexInfo::isGeoHaystack() #1085

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.

Sign up for GitHub

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 3 commits into from
Jun 13, 2023
Merged

PHPLIB-1057: Deprecate method IndexInfo::isGeoHaystack() #1085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36a345c
Deprecate method IndexInfo::isGeoHaystack()
GromNaN May 30, 2023
7dc5d33
Fix CS
GromNaN Jun 1, 2023
5182c1e
Merge branch 'master' into PHPLIB-1057
GromNaN Jun 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions docs/reference/method/MongoDBModelIndexInfo-isGeoHaystack.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ MongoDB\\Model\\IndexInfo::isGeoHaystack()

.. versionadded:: 1.4

.. deprecated:: 1.16
MongoDB 5.0 and later no longer supports geoHaystack indexes.
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 doc was updated in #1085 (comment) to mention the feature deprecation. I moved this note to a deprecation block.


.. default-domain:: mongodb

.. contents:: On this page
Expand All @@ -24,10 +27,6 @@ Definition

function isGeoHaystack(): boolean

.. note::

MongoDB 5.0 and later no longer supports geoHaystack indexes.

Return Values
-------------

Expand Down
6 changes: 6 additions & 0 deletions src/Model/IndexInfo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@

use function array_key_exists;
use function array_search;
use function trigger_error;

use const E_USER_DEPRECATED;

/**
* Index information model class.
Expand Down Expand Up @@ -124,9 +127,12 @@ public function is2dSphere()
* Return whether or not this index is of type geoHaystack.
*
* @return boolean
* @deprecated Since 1.16: MongoDB 5.0 removes support for geoHaystack indexes.
*/
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);
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@GromNaN GromNaN Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (isset($options['autoIndexId'])) {
trigger_error('The "autoIndexId" option is deprecated and will be removed in a future release', E_USER_DEPRECATED);
}

Starting in MongoDB 4.0, you cannot set the option autoIndexId to false when creating collections in databases other than the local database.

Copy link
Member

Choose a reason for hiding this comment

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

The @deprecated annotation should be enough to trigger the cleanup in 2.0

Makes sense.

trigger_deprecated is used for autoIndexId option in MongoDB\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.

Copy link
Member Author

Choose a reason for hiding this comment

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

We decided to keep the deprecation trigger.


return array_search('geoHaystack', $this->getKey(), true) !== false;
}

Expand Down
4 changes: 3 additions & 1 deletion tests/Model/IndexInfoFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ public function testIsGeoHaystack(): void
$index = $result->current();

$this->assertEquals($indexName, $index->getName());
$this->assertTrue($index->isGeoHaystack());
$this->assertDeprecated(function () use ($index): void {
Copy link
Member

Choose a reason for hiding this comment

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

I forgot this method existed. It dates back to 4b4a7d9#diff-abf1fa77a19f720052b75bc802df0260157b9064d8c3c12a80794f01e9acc065R136.

Copy link
Member Author

@GromNaN GromNaN Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

$this->assertTrue($index->isGeoHaystack());
});
$this->assertEquals(5, $index['bucketSize']);
}

Expand Down
28 changes: 21 additions & 7 deletions tests/Model/IndexInfoTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ public function testBasicIndex(): void
$this->assertSame('x_1', $info->getName());
$this->assertSame('foo.bar', $info->getNamespace());
$this->assertFalse($info->is2dSphere());
$this->assertFalse($info->isGeoHaystack());
$this->assertDeprecated(function () use ($info): void {
$this->assertFalse($info->isGeoHaystack());
});
$this->assertFalse($info->isSparse());
$this->assertFalse($info->isText());
$this->assertFalse($info->isTtl());
Expand All @@ -44,7 +46,9 @@ public function testSparseIndex(): void
$this->assertSame('y_sparse', $info->getName());
$this->assertSame('foo.bar', $info->getNamespace());
$this->assertFalse($info->is2dSphere());
$this->assertFalse($info->isGeoHaystack());
$this->assertDeprecated(function () use ($info): void {
$this->assertFalse($info->isGeoHaystack());
});
$this->assertTrue($info->isSparse());
$this->assertFalse($info->isText());
$this->assertFalse($info->isTtl());
Expand All @@ -66,7 +70,9 @@ public function testUniqueIndex(): void
$this->assertSame('z_unique', $info->getName());
$this->assertSame('foo.bar', $info->getNamespace());
$this->assertFalse($info->is2dSphere());
$this->assertFalse($info->isGeoHaystack());
$this->assertDeprecated(function () use ($info): void {
$this->assertFalse($info->isGeoHaystack());
});
$this->assertFalse($info->isSparse());
$this->assertFalse($info->isText());
$this->assertFalse($info->isTtl());
Expand All @@ -88,7 +94,9 @@ public function testTtlIndex(): void
$this->assertSame('z_unique', $info->getName());
$this->assertSame('foo.bar', $info->getNamespace());
$this->assertFalse($info->is2dSphere());
$this->assertFalse($info->isGeoHaystack());
$this->assertDeprecated(function () use ($info): void {
$this->assertFalse($info->isGeoHaystack());
});
$this->assertFalse($info->isSparse());
$this->assertFalse($info->isText());
$this->assertTrue($info->isTtl());
Expand Down Expand Up @@ -166,7 +174,9 @@ public function testIs2dSphere(): void
$this->assertSame('pos_2dsphere', $info->getName());
$this->assertSame('foo.bar', $info->getNamespace());
$this->assertTrue($info->is2dSphere());
$this->assertFalse($info->isGeoHaystack());
$this->assertDeprecated(function () use ($info): void {
$this->assertFalse($info->isGeoHaystack());
});
$this->assertFalse($info->isSparse());
$this->assertFalse($info->isText());
$this->assertFalse($info->isTtl());
Expand All @@ -187,7 +197,9 @@ public function testIsGeoHaystack(): void
$this->assertSame('pos2_geoHaystack_x_1', $info->getName());
$this->assertSame('foo.bar', $info->getNamespace());
$this->assertFalse($info->is2dSphere());
$this->assertTrue($info->isGeoHaystack());
$this->assertDeprecated(function () use ($info): void {
$this->assertTrue($info->isGeoHaystack());
});
$this->assertFalse($info->isSparse());
$this->assertFalse($info->isText());
$this->assertFalse($info->isTtl());
Expand All @@ -208,7 +220,9 @@ public function testIsText(): void
$this->assertSame('title_text_description_text', $info->getName());
$this->assertSame('foo.bar', $info->getNamespace());
$this->assertFalse($info->is2dSphere());
$this->assertFalse($info->isGeoHaystack());
$this->assertDeprecated(function () use ($info): void {
$this->assertFalse($info->isGeoHaystack());
});
$this->assertFalse($info->isSparse());
$this->assertTrue($info->isText());
$this->assertFalse($info->isTtl());
Expand Down