Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

use mongodb extension instead of mongo extension #34

Merged
merged 1 commit into from
Apr 11, 2016

Conversation

sandrokeil
Copy link
Contributor

This is a PR for #33

  • add option to use expireAfterSeconds index, default disabled
  • add install mongodb extension on Travis-CI

@sandrokeil sandrokeil force-pushed the feature/mongodb branch 2 times, most recently from b654897 to 8dee63a Compare March 8, 2016 21:40
@sandrokeil
Copy link
Contributor Author

@weierophinney All tests are green now. :-) Should I rename the class MongoDB to MongoDb?

@sandrokeil
Copy link
Contributor Author

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')) {
Copy link
Member

Choose a reason for hiding this comment

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

* add option to use expireAfterSeconds index, default disabled
* add install mongodb extension on Travis-CI
@sandrokeil
Copy link
Contributor Author

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.

@weierophinney
Copy link
Member

@sandrokeil 👍

"require": {
    "ext/mongodb": "*"
}

will fail if the extension is not present (however, it will not fail if you use the --ignore-platform-reqs flag, but if you do that, you're setting yourself up for problems anyways).

@weierophinney
Copy link
Member

Okay, just finally figured out what you meant by "there is no Composer constraint": the composer.json does not contain one currently because this is optional functionality.

As such, we have three options:

  • indicate in the suggest section that users should install ext/mongodb in order to use the MongoDB save handler. This, of course, breaks functionality for users who were previously using ext/mongo when they upgrade.
  • do the above, but also raise an exception in the MongoDB save handler if ext/mongodb is not loaded; the exception message should indicate what extension is needed, and could also indicate how to downgrade if the exception has occurred since an upgrade.
  • release the updated functionality as a separate package, and keep the existing functionality. We would then document to users that if they want the updated code or want to use ext/mongodb, they should also install the new package. If we go this route, we should also mark the current save handler as deprecated, and likely raise an E_USER_DEPRECATED from its constructor to signal users to start using the new package. This would allow adding the extension constraint to the new package's composer.json.

Pinging @ezimuel and @zendframework/community-review-team for feedback.

@weierophinney
Copy link
Member

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.

@sandrokeil
Copy link
Contributor Author

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;
Copy link
Contributor

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.

Copy link
Member

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.

@sandrokeil
Copy link
Contributor Author

@prolic Please take a look at this comment. But yes, we can also use the raw driver instead of the library.

@weierophinney weierophinney merged commit 70be000 into zendframework:develop Apr 11, 2016
weierophinney added a commit that referenced this pull request Apr 11, 2016
use mongodb extension instead of mongo extension
weierophinney added a commit that referenced this pull request Apr 11, 2016
weierophinney added a commit that referenced this pull request Apr 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants