PHPLIB-1680: Remove pre-4.2 wire version checks and test code #1801
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2.x #1801 +/- ##
============================================
+ Coverage 87.72% 87.74% +0.01%
+ Complexity 3195 3185 -10
============================================
Files 424 424
Lines 6296 6289 -7
============================================
- Hits 5523 5518 -5
+ Misses 773 771 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
GromNaN
left a comment
GromNaN
left a comment
There was a problem hiding this comment.
There is an error with PHPUnit tests (ubuntu-22.04, 8.1, 6.0, sharded_cluster)
UnexpectedValueException: Could not determine server storage engine
alcaeus
left a comment
alcaeus
left a comment
There was a problem hiding this comment.
One nit, and a request to run phpcbf. Changes themselves LGTM.
| /* Server versions >= 4.2.0 raise errors for unsupported update options. | ||
| * For previous versions, the CRUD spec requires a client-side error. */ | ||
| if (isset($this->options['hint']) && ! server_supports_feature($server, self::WIRE_VERSION_FOR_UNSUPPORTED_OPTION_SERVER_SIDE_ERROR)) { | ||
| throw UnsupportedException::hintNotSupported(); |
There was a problem hiding this comment.
Nit: Is this method still used after we remove everything? Not sure if we consider this public API, but we can remove the method if we consider it private and it's no longer used.
There was a problem hiding this comment.
It's still used by the Delete operation: https://github.com/mongodb/mongo-php-library/blob/2.1.2/src/Operation/Delete.php#L126-L133
I don't feel strongly about removing it after we raise the minimum server version to 4.4+, but I'm amenable to leaving it around and deprecated since it's technically public.
This logic appears to date back to 8abb006 and was never updated when later server versions added support on mongos.
jmikola
force-pushed
the
phplib-1680-rebase
branch
from
e3ffa90 to
657f785
Compare
| if ($this->isShardedCluster()) { | ||
| $this->markTestSkipped('Transactions are only supported on FCV 4.2 or higher'); | ||
| } | ||
| case Server::TYPE_RS_PRIMARY: |
There was a problem hiding this comment.
The condition on $this->getPrimaryServer()->getType() === Server::TYPE_RS_PRIMARY. I read the following comment, but I don't get the reason of this update.
There was a problem hiding this comment.
Previous behaviour was to skip on a standalone server, on sharded clusters (not sure why this was unconditional, it should've only skipped on sharded clusters < 4.2), and on replica sets with a storage engine other than WiredTiger. Since we're now always on 4.2 and higher, we can skip on standalones, check for WiredTiger on a primary, and always support sharded clusters, hence the change.
4 participants
https://jira.mongodb.org/browse/PHPLIB-1680
Supersedes #1695