What about adding that it requires an external lib?

The pattern public static function isSupported(): bool has been added to MongoDbStore as well as @requires extension mongodb on the class. I've also moved the CAUTION: phpdoc up to the class to be inline with other stores.

imho this is not needed

I did this because some of Jérémy's stores do it. I thought it might be useful later if a bundle extension needs to check support to auto default/configure something supported, but maybe we could add an interface for this later to all stores if we ever did something like that. Also thinking about it more... you could never auto configure a MongoDbStore since the database name option is mandatory. So I have removed static::isSupported().

You can directly check if it is supported here.

done

you should not type-hint the mongodb client, what happens if he does not exist ?

Then the developer wouldn't be able to construct a MongoDB\Client. If they pass something that isn't a MongoDB\Client they would get a TypeError:

Uncaught TypeError: Argument 1 passed to Symfony\Component\Lock\Store\MongoDbStore::__construct() must be an instance of MongoDB\Client, x given

This is what I would want to happen

What happens if you don't have MongoDB ?

if you don't have mongodb/mongodb you can't construct a MongoDB\Client

if you don't have ext-mongodb you can't composer require mongodb/mongodb

if you somehow have installed mongodb/mongodb and don't have ext-mongodb you will fail to construct a MongoDB\Client (this is mongodb/mongodb's responsibility):

<?php

require __DIR__.'/vendor/autoload.php';

$mongoClient = new MongoDB\Client('mongodb://127.0.0.1');
$store = new Symfony\Component\Lock\Store\MongoDbStore($mongoClient, [
    'database' => 'test',
]);

$ docker run --rm -it -v $PWD:$PWD -w $PWD php php ./test.php

Fatal error: Uncaught Error: Class 'MongoDB\Driver\Manager' not found in /home/kralos/src/symfony/vendor/mongodb/mongodb/src/Client.php:87
Stack trace:
#0 /home/kralos/src/symfony/test.php(5): MongoDB\Client->__construct('mongodb://127.0...')
#1 {main}
  thrown in /home/kralos/src/symfony/vendor/mongodb/mongodb/src/Client.php on line 87

MongoDB\Driver\Manager is provided by ext-mongodb

MongoDB\Client is provided by mongodb/mongodb

Symfony\Component\Lock\Store\MongoDbStore depends on mongodb/mongodb

I think you not type-hint MongoDB\Client for that reason. maybe i'm wrong cc @jderusse

I removed the if (!\extension_loaded('mongodb')) { check in the constructor since it's impossible to hit it. You can't pass a MongoDB\Client without mongodb/mongodb which you can't have without ext-mongodb.

We still have in the class docblock @requires extension mongodb

you should inline this

done

you should inline this

done

This works by accident, version_compare should be used for these

>>> '2.10' < '2.2'
=> true

instances: 2

But why are we adding support for legacy version anyway? 2.2 was EOL-ed in 2014. We are paying the price not just with added complexity but with extra query on each request as well.

Yeah, I'd prefer not to support <2.2. removing support and documenting requirements

This is inconsistent. IMHO parameter should either be non-optional, or not specified here

agreed, I've opted for not optional. the objective of the function is to create a MongoDb compliant UTCDateTime to millisecond precision.

Minor thing, but else branch could be easily avoided here by specifying this as default. This is super cheap call

updated

revert

fixed, something funny happened with my rebase

remove " in "%f" given too (for consistency)

updated

database and collection names could be defined in the DSN (similar to PDO). dsn = mongodb://[username:password@]host1[:port1][,host2[:port2:],...]/[database/][collection] ?

The database you see there is the authentication database, not the database to use for data

isn't it the purpose of the querystring parameter authSource? https://symfony.com/doc/master/bundles/DoctrineMongoDBBundle/installation.html#authentication

anyway, if the path component is a standard to declare the auth Database, we can use custom queryString parameters like mongodb://localhost/?lockDatabase=foo&lockCollection=bar

If we can not create the Store from a DSN, developpers using the FrameworkBundle won't be able to use it from the configuration framework.lock = '%env(LOCK_DNS)% (see symfony/symfony-docs#12523)

isn't it the purpose of the querystring parameter authSource?

Actually I just tested this (mongodb 4.2.0) and if both /path and ?authSource are specified /path means application database and ?authSource means authentication database. We are still stuck for getting the collection name.

we can use custom queryString parameters

Seems to be the only way, do you have any objection to us taking ?collection=bar? I feel like if we ever had a conflict it would be because MongoDB is using this param for the exact same purpose.

I'm going to enforce when $mongo is a:

• MongoDB\Collection:
  • $options['database'] is ignored
  • $options['collection'] is ignored
• MongoDB\Client:
  • $options['database'] is mandatory
  • $options['collection'] is mandatory
• MongoDB Connection URI string:
  • /path is used otherwise $options['database'], at least one is mandatory
  • ?collection= is used otherwise $options['collection'], at least one is mandatory

what's about something similare to Messenger PDO

if (false === $parsedUrl = parse_url($dsn)) {
    throw new InvalidArgumentException(sprintf('The given Mongodb DSN "%s" is invalid.', $dsn));
}

$query = [];
if (isset($parsedUrl['query'])) {
    parse_str($parsedUrl['query'], $query);
}
$this->option = $option + $query;
$this->options['database'] = $this->options['database'] ?? ltrim($parsedUrl['path'] ?? '', '/') ?: null;

You prefer $options to take precedence over /path?collection=?

<?php

declare(strict_types=1);

$mongo = 'mongodb://127.0.0.1/uri-db?collection=uri-col';
$options = [
    'database' => 'option-db',
    'collection' => 'option-col',
];

if (false === $parsedUrl = parse_url($mongo)) {
    throw new \InvalidArgumentException(sprintf('The given MongoDB Connection URI "%s" is invalid.', $mongo));
}
$query = [];
if (isset($parsedUrl['query'])) {
    parse_str($parsedUrl['query'], $query);
}
$options += $query;
$options['database'] = $options['database'] ?? ltrim($parsedUrl['path'] ?? '', '/') ?: null;

print_r($options);
/* Array
(
    [database] => option-db
    [collection] => option-col
) */

I've implemented this but without polluting $this->options with querystring params other than collection.

if (false === $parsedUrl = parse_url($mongo)) {
    throw new InvalidArgumentException(sprintf('The given MongoDB Connection URI "%s" is invalid.', $mongo));
}
$query = [];
if (isset($parsedUrl['query'])) {
    parse_str($parsedUrl['query'], $query);
}
$this->options['collection'] = $this->options['collection'] ?? $query['collection'] ?? null;
$this->options['database'] = $this->options['database'] ?? ltrim($parsedUrl['path'] ?? '', '/') ?: null;