-
Notifications
You must be signed in to change notification settings - Fork 62
use mongodb extension instead of mongo extension #34
use mongodb extension instead of mongo extension #34
Conversation
b654897
to
8dee63a
Compare
@weierophinney All tests are green now. :-) Should I rename the class MongoDB to MongoDb? |
Before merge, take a look at the discussion in the issue #33 for this PR. Maybe there is additional work required. |
@@ -43,8 +44,10 @@ class MongoDBTest extends \PHPUnit_Framework_TestCase | |||
*/ | |||
public function setUp() | |||
{ | |||
if (!extension_loaded('mongo')) { | |||
$this->markTestSkipped('Zend\Session\SaveHandler\MongoDB tests are not enabled due to missing Mongo extension'); | |||
if (!extension_loaded('mongodb')) { |
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.
I think we should go rid of this check and use @requires
annotations https://phpunit.de/manual/current/en/incomplete-and-skipped-tests.html#incomplete-and-skipped-tests.requires.tables.api
8dee63a
to
a4ecc3a
Compare
* add option to use expireAfterSeconds index, default disabled * add install mongodb extension on Travis-CI
a4ecc3a
to
70be000
Compare
Thanks for clarification. I've updated the PR to restore BC for version 2.7. One problem is how to ensure that people have to use the mongodb extension. As mentioned by @Maks3w there is no Composer constraint. |
"require": {
"ext/mongodb": "*"
} will fail if the extension is not present (however, it will not fail if you use the |
Okay, just finally figured out what you meant by "there is no Composer constraint": the As such, we have three options:
Pinging @ezimuel and @zendframework/community-review-team for feedback. |
Just a quick note: separation to a new package is the route that @marc-mabe is planning for storage adapters in zend-cache, for exactly this same reason. |
Yes, separation for each special save handler, which relies on dependencies, in an extra package is the best choice. I'm for option 3 and waiting for the feedback. |
use MongoDate; | ||
use MongoDB\BSON\Binary; | ||
use MongoDB\BSON\UTCDatetime; | ||
use MongoDB\Client as MongoClient; |
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.
It's only available in mongo php library. We don't need this. We can do everything with the raw extension here, imho.
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.
We can, but it requires far more code and code complexity. The Mongo PHP Library exists to provide the convenience layer that was previously in the extension, but in a way that can be more quickly and easily updated as new features are added and/or bugfixes released. Considering most developers will now be using the extension / PHP library combination if using MongoDB at all on PHP, this is not an unreasonable requirement.
@prolic Please take a look at this comment. But yes, we can also use the raw driver instead of the library. |
use mongodb extension instead of mongo extension
This is a PR for #33