Skip to content

Remove deprecated functionality #1439

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

Jump to bottom
Merged
merged 8 commits into from
Sep 30, 2024
Merged

Remove deprecated functionality #1439

merged 8 commits into from
Sep 30, 2024

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Sep 23, 2024 •
edited
Loading

This PR includes:

  • PHPLIB-360: make ChangeStream::CURSOR_NOT_FOUND private
  • PHPLIB-818: remove Watch::FULL_DOCUMENT_DEFAULT
  • PHPLIB-1512: remove the deprecated IndexInfo::isGeoHaystack method
  • PHPLIB-1511: remove the deprecated maxScan, modifiers, oplogReplay, and snapshot query options for find and findOne
  • PHPLIB-1517: remove the MonogDB\Operation\Executable interface
  • PHPLIB-1523: remove iterators for databases, collections, and indexes
  • PHPLIB-1513: remove mapReduce functionality

@alcaeus alcaeus force-pushed the 2.0-remove-things branch 2 times, most recently from 9bd1347 to 43a304d Compare September 25, 2024 07:45
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 25, 2024
View reviewed changes
@alcaeus alcaeus force-pushed the 2.0-remove-things branch 4 times, most recently from 21a2ef8 to 4edfa16 Compare September 26, 2024 10:32
@alcaeus alcaeus marked this pull request as ready for review September 26, 2024 10:33
@alcaeus alcaeus requested a review from a team as a code owner September 26, 2024 10:33
@alcaeus alcaeus requested a review from jmikola September 26, 2024 10:33
GromNaN
GromNaN approved these changes Sep 26, 2024
View reviewed changes
alcaeus
alcaeus commented Sep 26, 2024
View reviewed changes
tests/UnifiedSpecTests/Operation.php
$args['out'],
array_diff_key($args, ['map' => 1, 'reduce' => 1, 'out' => 1]),
));
Assert::markTestSkipped('mapReduce operation is not supported');
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, there's no markTestSkipped helper function like the various assert helpers, hence the static call in the assert class. The end result wouldn't differ, as this method only throws an exception. I figured that marking a test as skipped when it uses mapReduce makes more sense than explicitly listing all tests. We currently list some tests explicitly when we don't support the tested operations, and we can consider moving those to the operation class as well to reduce the size of our skip list.

Copy link
Member

Choose a reason for hiding this comment

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

In the past, my only concern with skipping at this point would be that we're already midway into the test. I trust this is safer now that we have a finally block for some cleanup around operation execution: 90dcf18#diff-fab05380d19ea3d7a426128a38cde0e262291e90b82294c085a7397d949216a8R216

I suppose there's some wasted effort running the test until this point, but if that makes the skip list easier to digest I suppose it's worth the trade-off.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, ideally all kinds of cleanup happens in finally blocks to make sure we clean up. That said, I currently don't see any negative side effects, so it looks like we've done things properly, as I'd expect 😉

jmikola
jmikola reviewed Sep 26, 2024
View reviewed changes
tests/Model/IndexInfoTest.php
]);

$this->assertSame(2, $info->getVersion());
$this->assertSame(['pos' => '2dsphere'], $info->getKey());
$this->assertSame('pos_2dsphere', $info->getName());
$this->assertSame('foo.bar', $info->getNamespace());
Copy link
Member

Choose a reason for hiding this comment

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

How did this work previously if getNamespace() was already emitting E_USER_DEPRECATED?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an excellent question 🤷‍♂️

tests/UnifiedSpecTests/Operation.php
$args['out'],
array_diff_key($args, ['map' => 1, 'reduce' => 1, 'out' => 1]),
));
Assert::markTestSkipped('mapReduce operation is not supported');
Copy link
Member

Choose a reason for hiding this comment

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

In the past, my only concern with skipping at this point would be that we're already midway into the test. I trust this is safer now that we have a finally block for some cleanup around operation execution: 90dcf18#diff-fab05380d19ea3d7a426128a38cde0e262291e90b82294c085a7397d949216a8R216

I suppose there's some wasted effort running the test until this point, but if that makes the skip list easier to digest I suppose it's worth the trade-off.

@jmikola jmikola mentioned this pull request Sep 26, 2024
Merged
@alcaeus alcaeus requested a review from jmikola September 27, 2024 06:54
GromNaN
GromNaN reviewed Sep 27, 2024
View reviewed changes
tests/Operation/FindFunctionalTest.php Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change after rebase: #1412 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

It has been removed along with the entire test surrounding it.

@alcaeus alcaeus requested a review from GromNaN September 30, 2024 07:31
@alcaeus alcaeus merged commit 3d46eaf into mongodb:v2.x Sep 30, 2024
31 checks passed
alcaeus added a commit that referenced this pull request Oct 16, 2024
@alcaeus
Merge branch 'v2.x' into merge-v1.x-into-v2.x-1729063221979
bfc165a 
* v2.x:
  PHPLIB-1546 and PHPLIB-1159: Remove CreateCollection flags and autoIndexId options (#1478)
  PHPLIB-1227 Use void return types for operations without meaningful result document (#1468)
  Remove deprecated functionality (#1439)
  PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result (#1454)
  PHPLIB-954: Add return types to all methods (#1391)
  PHPLIB-797: Remove unused methods in UnsupportedException (#1436)
  Revert "Add final annotations to non-internal Operation classes (#1410)"
  PHPLIB-953 Make internal classes and Operation classes final (#1392)
  PHPLIB-1218 Remove deprecated fields from GridFS files (#1398)
alcaeus added a commit that referenced this pull request Oct 29, 2024
@alcaeus
Merge branch 'v2.x' into merge-v1.x-into-v2.x-1730186279994
4248f52 
* v2.x:
  PHPLIB-1546 and PHPLIB-1159: Remove CreateCollection flags and autoIndexId options (#1478)
  PHPLIB-1227 Use void return types for operations without meaningful result document (#1468)
  Remove deprecated functionality (#1439)
  PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result (#1454)
  PHPLIB-954: Add return types to all methods (#1391)
  PHPLIB-797: Remove unused methods in UnsupportedException (#1436)
  Revert "Add final annotations to non-internal Operation classes (#1410)"
  PHPLIB-953 Make internal classes and Operation classes final (#1392)
  PHPLIB-1218 Remove deprecated fields from GridFS files (#1398)
alcaeus added a commit that referenced this pull request Nov 4, 2024
@alcaeus
Merge branch 'v2.x' into merge-v1.x-into-v2.x-1730397444425
0870eaf 
* v2.x:
  Regenerate evergreen configuration (#1503)
  PHPLIB-1546 and PHPLIB-1159: Remove CreateCollection flags and autoIndexId options (#1478)
  PHPLIB-1227 Use void return types for operations without meaningful result document (#1468)
  Remove deprecated functionality (#1439)
  PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result (#1454)
  PHPLIB-954: Add return types to all methods (#1391)
  PHPLIB-797: Remove unused methods in UnsupportedException (#1436)
  Revert "Add final annotations to non-internal Operation classes (#1410)"
  PHPLIB-953 Make internal classes and Operation classes final (#1392)
  PHPLIB-1218 Remove deprecated fields from GridFS files (#1398)
alcaeus added a commit that referenced this pull request Feb 28, 2025
@alcaeus
Merge branch 'v2.x' into merge-v1.x-into-v2.x-1740741263182
09777ea 
* v2.x:
  Add return type hint for Encoder::encode() implementation
  Reverse pipeline init from an array (#1596)
  PHPLIB-1617 Accept a Pipeline instance in aggregate and watch methods (#1580)
  Fix CS
  Require latest python version (#1564) (#1565)
  Update src/Operation/Find.php
  Ignore `disableMD5` option as `md5` field is removed from the spec (#1502)
  Remove obsolete baseline entries
  Regenerate evergreen configuration (#1503)
  PHPLIB-1546 and PHPLIB-1159: Remove CreateCollection flags and autoIndexId options (#1478)
  PHPLIB-1227 Use void return types for operations without meaningful result document (#1468)
  Remove deprecated functionality (#1439)
  PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result (#1454)
  PHPLIB-954: Add return types to all methods (#1391)
  PHPLIB-797: Remove unused methods in UnsupportedException (#1436)
  Revert "Add final annotations to non-internal Operation classes (#1410)"
  PHPLIB-953 Make internal classes and Operation classes final (#1392)
  PHPLIB-1218 Remove deprecated fields from GridFS files (#1398)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmikola jmikola jmikola approved these changes

@GromNaN GromNaN GromNaN approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alcaeus @jmikola @GromNaN