PHP 7 driver support

Yes, but only for the 8.x versions. I discussed this with jmikola (the main driver author, who wrote that post) when he came to France some weeks ago, and he advised against redoing our module on top of the new driver for existing versions, but rather use the new driver, for php7, as the foundation of our 8.x versions. He also suggested that we would probably be better off using just the driver lower layer (C extension), not the user-space layer, since we are essentially building a user-space driver of our own targeted to our needs.

You mean on D7 or D8 ? Because D7 itself doesn't support PHP7 already, and the D8 version of the module is not supposed to be working at all on D8.0.1.

Tagging for Milan code sprint

Committed to the 8.x-2.x branch yesterday: https://github.com/FGM/mongodb/commit/a895cde4ec21628a70c127645788ac7f94...

We can reopen this if someone actually wants this on 8.x-1.x.

Fixed in 8.x-2.x, not 8.x-1.x.

I think over time more people will want to use Drupal 7 with PHP 7.

For me this is currently occuring because I installed a new Linux (Mint 18.1), where PHP 7 is the default you get from the repositories.
I thought I can either downgrade PHP to 5.6, or I can see if I can get Drupal 7 to work with PHP 7. I chose the latter, for now.
(I guess I could also use docker containers or something.. but then I would have to reorganize a lot of stuff on my local dev machine)

The mongodb module for Drupal 7 fails the requirements check on PHP 7, because the 'mongo' extension does not exist in PHP 7.
Instead, it is now called 'mongodb' extension, and has some classes changed.
http://stackoverflow.com/questions/34486808/installing-the-php-7-mongodb...
(which you already know, I suppose. Just saying it for other readers.)

@fgm
I updated the issue summary based on my limited knowledge. Feel free to correct any mistake.

If support for the new driver is not added in PHP 7, we should add least prevent it from crashing.

Steps to reproduce a crash:
- Install mongodb and some submodules (e.g. mongodb_watchdog) on a machine with PHP 5.* and 'mongo' extension.
- Copy the site's code + database to a machine with PHP 7.* and 'mongodb' extension. Or update the existing machine to use PHP 7 and mongodb extension.
-> profit! (class 'Mongo' not found in function mongodb())

With this crash going on, it is not even possible to disable the module.

I suppose this only happens if something does trigger mongodb(). In my case it was the watchdog.

So here is what could be done:
- check if class Mongo exists, throw an exception otherwise.
- catch exceptions in strategic places in code. Typically at the point of integration. E.g. in mongodb_watchdog_watchdog().

Could you submit a patch for this ? D7 is quite "legacy" these days, so I'm not totally convinced to spend time on this in when projects tend to be launched on 8.3 core anyway.

Reopening and retagging for D7 since this is what this issue is now about. No longer about DDD Milan, either.

I'll add my voice for support of this. The sites and clients I work with won't be migrating to Drupal 8 any time soon, and the old PHP Mongo driver is getting left in the dust. I would imagine at some point the old driver won't be compatible with newer versions of Mongo (though I've had luck with versions up to 3.2).

I'll add this won't just be helpful for PHP 7, but should also be helpful for PHP 5, since the newer mongodb driver works with both.

I'd be happy to help out with patches, as well.

I'll be glad to review any patches to D7 components except for field storage and blocks, which I never touched anyway, so feel free to submit them. Patches should have tests.

when projects tend to be launched on 8.3 core anyway.

I have often added mongodb to existing D7 projects for performance.
So this does not only apply to newly launched projects.

This does not mean you (fgm) are obliged to anything, it just means your assumption about the need might be inaccurate.

I have tried following the instructions from the link in #14 but I'm getting the Currently using Mongodb Not found error when I try to enable the module. Using php7.1

@donquixote @majorrobot: yes, adding MongoDB to D7 is indeed currently way more common than including it to a D8 project, no doubt about that. That's why I have no objection of any kind to reviewing and committing D7 patches if anyone want to submit them. It's just that I haven't had a "new site" D7 project since 2014: only D8, so I don't plan on doing these patches myself.

@jbrandl07: it is not necessarily related to your issue but AFAIK PHP 7.1 is not entirely compatible with Drupal 8.0/8.1/8.2 and probably 8.3. Supported versions are 5.5.x (x >= 9), 5.6.x, and 7.0.x only: there are issues with the breaking changes in the assert() system with 7.1.

I modified (in my local version of the module) the hook_requirements() to stop complaining if the old library is missing.
And wrote a separate module that requires the mongo-php-adapter with composer_manager.

The next problem I run into:
MongoDb::getCollectionNames() seems to have a different signature depending where you look, or depending which version of PHP or of the extension or whatever.

• In http://php.net/manual/en/mongodb.getcollectionnames.php it says the parameter type is array: public array MongoDB::getCollectionNames ([ array $options = array() ] ).
• In the mongo-php-adpater it also requires an array value as the parameter.
• The Drupal 7 module calls it with FALSE, so a boolean.
• The PhpStorm stub specifies the signature as public function getCollectionNames($includeSystemCollections = false) {}, with boolean parameter.

This results in

TypeError: Argument 1 passed to MongoDB::getCollectionNames() must be of the type array, boolean given, called in /.../sites/all/modules/contrib/mongodb/mongodb_cache/mongodb_cache_module.php on line 47 in MongoDB->getCollectionNames() (line 167 of /.../sites/all/vendor/alcaeus/mongo-php-adapter/lib/Mongo/MongoDB.php).

Unfortunately I don't find documentation on the old signature, other than in PhpStorm. 3v4l.org is no help either, because it does not support extensions.

But omitting the parameter should be good enough.
With the old signature it defaults to getCollectionNames($includeSystemCollections = false).
With the new signature it defaults to getCollectionNames($options = ['includeSystemCollections' => false, ..]).

This patch contains the changes I did to the module on my localhost.
It does not contain additional tests. I don't expect it to be committed as is, just want to share my progress so others can continue from there.
I originally created mongophpadapter as a separate module, but I think it is useful to ship it as a submodule with the main module package.

What this patch does:
- Add an (optional) submodule mongophpadapter that tells composer_manager to download the mongo-php-adapter library.
- Change hook_requirements() to accept the replacement for the old ext-mongo library, and give detailed feedback if something is missing.
- Omit the explicit parameter in the call to MongoDB::getCollectionNames().

How to use:
- On PHP 7.x with ext-mongodb extension.
- Enable mongodb module and mongophpextension modules. The latter will ask you to download composer_manager, and will then install the mongo-php-adapter library from composer.
- Enable other submodules of mongodb as usual, and/or configure them in settings.php.

It does not work that way. Not that easy.

We need to launch composer_manager in settings.php.
I tried this by calling composer_manager_register_autoloader() in settings.php. But this indirectly triggers db_merge() via variable_set(). Which blows up because the database is not initialized yet. Too bad!

Ok, not such a problem after all.

In settings.php:

if (file_exists($composer_autoload_php = DRUPAL_ROOT . '/sites/all/vendor/autoload.php')) {
  require_once DRUPAL_ROOT . '/sites/all/vendor/autoload.php';
}

// MongoDB
if (class_exists('MongoClient')) {
  ..

The if() is for the case where drush composer-manager install has not been executed yet, and the vendor/* stuff does not exist yet.

Btw why can I not mark a patch for testing with 7.x-1.x?

For the record, composer_manager should not be necessary to use the module, at least when not using the new extension adapter. If it has to be used for that adapter library, then so be it (although I tend to prefer adding a project-level composer.json file even on D7 and handle the dependencies there manually instead of requiring an additional module), but it should not make the module required if that library is not used.

You cannot trigger tests on d.o. because the module needs MongoDB which is not provided by DrupalCI.

For the record, composer_manager should not be necessary to use the module, at least when not using the new extension adapter.

In the patch, composer_manager is only required by the submodule 'mongophpadapter'.
This needs better documentation, like a README.md, obviously.

although I tend to prefer adding a project-level composer.json file even on D7 and handle the dependencies there manually instead of requiring an additional module

So far composer_manager has always done the job for me, with several modules. If a lot of modules have composer dependencies, I imagine it quickly becomes unmanageable to keep your projects's composer.json up to date manually.

You cannot trigger tests on d.o. because the module needs MongoDB which is not provided by DrupalCI.

The d.o. tests would confirm that the patch applies and that Drupal installs properly. Which of course has nothing to do with the core purpose of the module. So I don't have a clear opinion. Maybe not having the tests on d.o. prevents the false comfort of a "green" patch where only some basics are tested. And reduces stress on the testing infrastructure.

About tests: yes, that was my logic back when I enabled CI on the module a few years ago. But then only a few unit tests were able to pass because most actually depended on having a mongo server available, so at some point it got turned back off.

@rhclayto : if you have a working version that works as well as the existing one on the legacy version and also works with PHP 7 and your IP policy enables you to publish it, you could submit the patch here for review.

@rhclayto, would you be willing to share your code?

Bumping to major, as PHP 5.6 is getting EOL-ed this year, so this is probably the most pressing issue for 7.x. Asked @rhclayto for his patch in private message too.

FWIW the mongofill project mentioned by @PaulB on #3 has now been archived with mutliple bugs since 2016, so it seems the only option is the alcaeus adapter found by @donquixote, which appears to still be maintained: https://github.com/alcaeus/mongo-php-adapter/

I've converted @donquixote's patch in #24 to a PR: https://github.com/fgm/mongodb/pull/39

For some reason, xautoload 7.x-5.7 doesn't succesfully autoload the alcaeus classes, even when registering the composerDir(), so I just relied the Composer autoloader, explaining how to do it in the README.

I think now that we work on github and can do pull requests instead of patches, we should split the changes into smaller commits.

I opened PR https://github.com/fgm/mongodb/pull/40 as a starting point for cherry-pick and rebase.
Atm this is just what I found on my local version of the module, from last time I worked on it.

#2985089: MongoDb::getCollectionNames() parameter: boolean or array? is the first part of the PR split into a separate issue for transparency's sake.

From github PR discussion, which we decided to continue back here on drupal.org for visibility:

Basically, in the last 5 years or so, I built all my D7 and D8 projects pretty much like drupal-composer/drupal-project, with a project root containing the composer files and vendor/, the logs, tmpdir, privatedir, settings, etc, and just adding a require_once '../vendor/autoload.php' to load all the composer dependencies, and building the non-drupal project dependencies entirely with composer and npm/yarn, not drush make, even for D7 (actually also converted my legacy D6 projects to it).

With this approach, just requiring drupal/mongodb and mongodb/mongodb is sufficient to have autoload working. In the case of this issue, I tried to use xautoload because it is less exotic than the approach above but it wouldn't load the \MongoClient class from ../vendor/alcaeus/mongodb-php-adapter, although the files are actually present in ../vendor/composer/autoload_*, but it doesn't matter since the composer autoloader does the job well enough, although it is probably less optimized.

To conclude my own summary of "ways to integrate Composer dependencies":

I think there are a few different strategies out there for composer integration in D7. The choice is made per project / website.

1. composer_manager is suitable for projects that started without composer integration, and allows to add Composer libraries without restructuring the project itself.
   This approach assumes a composer.json in each module that wants to declare Composer dependencies. Such a composer.json typically contains 3rd party dependencies, but no dependencies on Drupal module packages.
   I don't know if it is possible to manually add additional package dependencies this way.
2. Some new D7 projects install everything, including Drupal core itself and all contrib modules, with Composer.
   This approach assumes a composer.json in each module package (which may contain more than one module) that wants to declare Composer dependencies. Such composer.json should also contain other Drupal modules that are required for download.
   It is still possible to manually require additional packages, that are not explicitly required by any module or package.
3. Some projects (as @fgm explained) manage all Composer dependencies outside of Drupal root, and require all packages manually.
   This approach assumes no composer.json in the modules, but it means that a site builder / developer needs to require the correct packages in a suitable version manually each time they install or update a module.
4. xautoload can help you with class loading, but not with package management or dependency management. And it needs to know where your class files are, obviously. I usually don't use xautoload for this purpose, even though it is "my baby".

This means we have different approaches with different requirements or assumptions on module-provided composer.json files.
Ideally, the module should work with all of those approaches.

In our case, the composer.json for approach 1. and 2. would be mostly the same, if we want to always require mongo-php-adapter.
This is what the PR does by adding a composer.json, https://github.com/fgm/mongodb/pull/39/files#diff-b5d0ee8c97c7abd7e3fa29...

However, I should ask: Do we always need to require this library?
If someone uses the old ext-mongo extension, they don't need the mongo-php-adapter library, right?
It does not really hurt either.. but then we should change the text in the requirements perhaps.
Currently in the PR it says this:

The mongo-php-adapter library is installed while the old ext-mongo extension is present. This does not hurt, but is somewhat redundant.

It seems the current logic in the patch is a bit off: it now checks the server version only when using the legacy mongo extension.

The server version check is independent of the extension: findAndModify was only implemented in mongodb 1.3, because a lot of the code won't work without it, so that needs to be checked in all extension cases, although this is very outdated now that 4.0 is out.

Regarding autoloading, and considering users of the legacy mongo extensions don't need it, I think that we should not add it as a module dependency, but just ensure the alcaeus classes are autoloadable in hook_requirements if the requirements check concludes to a current mongodb extension deployment. The classes in the module itself are not autoloaded anyway, and I'd rather not touch this code at all.

Since the requirements check is getting a bit hairy, it would be nice to refactor it to a separate "service" with easily testable individual checks and a simple main entrypoint, as is done in 8.x-2.x for the watchdog requirements: https://github.com/fgm/mongodb/blob/8.x-2.x/modules/mongodb_watchdog/src...

@fgm. Would like to confirm my understanding so far.

1. I get that we need to split hook_requirements() to call 2 parts. One containing the checks done in PR https://github.com/fgm/mongodb/pull/40/files and another doing the server version check currently done in https://github.com/fgm/mongodb/blob/7.x-1.x/mongodb.install

2. As per your above comments I see that you are looking to make this into separate services rather than just functions. So should I use the Services module in Drupal 7.x to rather split this into 2 services as given in point 1 and call them in hook_requirements() or do you just mean that it is enough for both the requirements to be just separate functions?

Am asking the question in point 2 since I do not clearly get the requirement for writing this as service as this code is going to be used inside Drupal itself in hook_requirements(). But in case you have any other plan kindly briefly explain on same.

Sorry for the delay. But I hope it is not too late.

1. Indeed.
2. No, Services module is completely unrelated to services for dependency injection in the D8/Symfony sense: Services module about exposing web services, not about dependency injection. The closest equivalent to D8 services was at one point the service_container module, but it is now deprecated and replaced by the backport module and its dependencies, but we don't really need either : services are just configurable code that gets instantiated from its dependencies and uses these injected dependencies and configuration to perform its purpose ; it does not depend on having a service container (e.g. the Symfony DIC, or Pimple, or some other PSR-11 container) ; these containers are just a way to provide access to the services and make them configurable more easily.

@fgm Created PR https://github.com/fgm/mongodb/pull/54 to start with. Could you be more specific and give an example on the things we would need to make configurable in this code so I can follow the same ?

Review sent. Regarding the PR process, you should probably create a branch named like 2644482-php7 on your fork and do the PR from that branch to 7.x : it will be easier for you to continue following other 7.x issues if your 7.x matches the upstream 7.x instead of having extra commits.

@fgm I have done as you requested. I hope this is what you had in mind. Anyway feel free to let me know on further review comments.

https://github.com/fgm/mongodb/pull/55

Closed old PR since I have done this on a separate branch as you suggested

Review send. It's getting closer, indeed.

@fgm Updated PR #55

Hey guys, where are we at with this? What's still needed?

As Drupal 7.61 now fully supports PHP 7.2 and due to Ubuntu 14.04 going EOL in the coming months I see a lot of Drupal 7 projects being moved to 18.04 with PHP 7 - One I'm doing right now requires MongoDB and thus this module...

Would be great to get the D7 version of this module running on the php 7 mongodb extension.

@jarrodirwin : have you tried using that branch for your site, to validate if it works for you ?

@fgm ahh right, there is a github feature branch for this. Sorry, missed that.

Yep, can confirm that using the D7 code on the feature branch with a MongoDB 4.0.5, PHP7.2 (Ubuntu 18.04), the new mongodb PHP extension and the mongo-php-adapter package added via composer works fine.

Is there any reason this is being handled as a separate feature branch off github instead of the standard Drupal.org way of supplying patches in this issue queue?

What's needed to be able to get this feature branch merged in?

Yes, the project uses Drupal issue queue for tracking because it's best to find issues in the longer term, but github PR's instead of patches for two reasons:

• testability: it is not currently possible to run tests on q.d.o. if they need a non-standard resource like a MongoDB instance, while we can run tests on Travis from Github.
• overall greated simplicity as many patches change sets tend to be multiple tens of kB, making patches unreviewable

What is needed: mostly returns on experience like yours, and reviews on the PRs : can you review the latest PR by kswamy ?

Hi,

I confirm that the patch works with the following configuration :
PHP 7.0.14-1
MongoDB Extension 1.24
Mongo DB 2.6.7

Merged to 7.x-1.x HEAD. Thanks donquixote for the patch, and all who tested it.