Skip to content
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.

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

Use PHP ext mongodb instead of mongo for v3 #23

Open
weierophinney opened this issue Dec 31, 2019 · 10 comments
Open

Use PHP ext mongodb instead of mongo for v3 #23

weierophinney opened this issue Dec 31, 2019 · 10 comments

Comments

@weierophinney
Copy link
Member

The Zend\Session\SaveHandler\MongoDB should use the new MongoDB extension which runs also under PHP 7 and should not rely on the deprecated extension mongo which is not compatible with PHP 7.

I think it should based on the official MongoDB PHP library. We can add this library to the suggest Composer section.

What do you think?


Originally posted by @sandrokeil at zendframework/zend-session#33

@weierophinney
Copy link
Member Author

@weierophinney
Copy link
Member Author

I didn't try it but I think we should update the handler. I can bring a PR if it is accepted.


Originally posted by @sandrokeil at zendframework/zend-session#33 (comment)

@weierophinney
Copy link
Member Author

@sandrokeil - please do! Also, we should definitely target the official wrapper, and not arbitrary third party wrappers.


Originally posted by @weierophinney at zendframework/zend-session#33 (comment)

@weierophinney
Copy link
Member Author

Maybe it's better to not rely on mongo-php-library and use the low level functions of the MongoDB Driver. It's more work, but we have a dependency fewer. I will update my PR depending on the decision.


Originally posted by @sandrokeil at zendframework/zend-session#33 (comment)

@weierophinney
Copy link
Member Author

I prefer remove the support to the old mongo driver and focus the efforts only in the new mongodb extension. But this is not BC,

/fw @weierophinney


Originally posted by @Maks3w at zendframework/zend-session#33 (comment)

@weierophinney
Copy link
Member Author

@Maks3w ext/mongo has been deprecated, and, more importantly, does not support PHP 7. We need to migrate away from it.

Additionally, the page linked by @sandrokeil says specifically:

Application developers should consider using this extension in conjunction with the » MongoDB PHP library, which implements the same higher level APIs found in MongoDB drivers for other languages.

I'd argue the code in this component (and other ZF components) falls directly into that category. As such, the requirements should be:

  • ext/mongodb
  • mongodb/mongodb (at stability ^1.0)

Chances are, if a developer is using mongo already in their application, and using ext/mongodb, they will be using the MongoDB PHP library anyways, so this will not be an additional dependency for them, and will help simplify our own code.


Originally posted by @weierophinney at zendframework/zend-session#33 (comment)

@weierophinney
Copy link
Member Author

My point is we fall under the same scenario of removing zend-crypt from zend-mail.

If we remove the support for ext/mongo this should happen on next major version because composer won't alert about the change in the dependency. Old users must install the new extension.


Originally posted by @Maks3w at zendframework/zend-session#33 (comment)

@weierophinney
Copy link
Member Author

@Maks3w Not true; composer honors extension constraints. As such, if the extension is missing when you run composer install, it will fail. At that point, you would need to select a new constraint for zend-session (to install the older version), or install the new extension.


Originally posted by @weierophinney at zendframework/zend-session#33 (comment)

@weierophinney
Copy link
Member Author

There aren't required extensions defined https://github.com/zendframework/zend-session/blob/master/composer.json#L15-L18

And I guess we won't add mongodb because is only required for only one specific savehandler.

May we could try to add the old extension as a conflicted dependency. But many other libraries may require the old extension.


Originally posted by @Maks3w at zendframework/zend-session#33 (comment)

@weierophinney
Copy link
Member Author

What's about to use a class alias like in prior versions of stdlib?


Originally posted by @sandrokeil at zendframework/zend-session#33 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant