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

Fix mongo handler #6184

Merged
merged 2 commits into from
May 15, 2014
Merged

Fix mongo handler #6184

merged 2 commits into from
May 15, 2014

Conversation

Nainterceptor
Copy link
Contributor

Hello,

First of all, tests pass :

php run-tests.php Session
> OK, but incomplete or skipped tests!
> Tests: 568, Assertions: 781, Skipped: 1

This fix is about the last version of php-mongo driver, where safe option is deprecated.
More informations here : http://www.php.net/manual/fr/mongo.writeconcerns.php#mongo.writeconcerns.options

On my dev environnement (archlinux), a lot of "deprecated" messages are present without this fix.

@Ocramius Ocramius added this to the 2.3.2 milestone Apr 29, 2014
@Ocramius Ocramius self-assigned this May 12, 2014
Ocramius added a commit that referenced this pull request May 15, 2014
@Ocramius Ocramius merged commit 997791d into zendframework:master May 15, 2014
@moderndeveloperllc
Copy link
Contributor

@Ocramius @Nainterceptor The concern I have about this is that the change from safe to w happened with driver 1.3 which only came out a year and a half ago. Elsewhere in the code we are doing a lot of pre-1.3 code and post-1.3 code (mainly around Mongo() vs MongoClient()). I'm thinking instead of changing the class code, we would change the test code to take that into account. As it is, we fix some E_DEPRECATED notices in the tests at the expense of breaking the actual functionality of the class for those pre-1.3.

@Ocramius
Copy link
Member

@moderndeveloperllc can you open an issue on that? It would be a blocker for 2.3.2.
If we can simply have the version of the driver somehow, that would be the solution.

@moderndeveloperllc
Copy link
Contributor

@Ocramius #6281. What's the policy about having a __construct() on a class extending AbstractOptions?

@Ocramius
Copy link
Member

@moderndeveloperllc why not?

@moderndeveloperllc
Copy link
Contributor

@Ocramius None that I can think of other than the dislike of setting defaults in an Options constructor. I'll try and get to this soon unless you or @Nainterceptor get to it first.

@Ocramius
Copy link
Member

@moderndeveloperllc I don't have time for it today, but please open a bug report on it, as this is a blocker for 2.3.2

@jmikola
Copy link
Contributor

jmikola commented May 15, 2014

On a somewhat related note, was Zend/Log/Writer/MongoDB fine as-is, because it has no default write options? I believe the session adapter is the only one that had an options class.

I originally implemented both of these MongoDB drivers two years ago, when driver 1.3 was pretty new. At this point, I don't think it's worth supporting the 1.2.x branch at all (I intend to remove it from Doctrine MongoDB's next minor version), but I understand if you can't simply drop support for it in a ZF patch release.

@jmikola
Copy link
Contributor

jmikola commented May 15, 2014

FYI: For doing logic around the MongoDB driver, I would suggest using version_compare() with phpversion('mongo'), as we do in Doctrine MongoDB:

return version_compare(phpversion('mongo'), '1.3.0', '<')
    ? new \Mongo($server, $options)
    : new \MongoClient($server, $options);

Source: https://github.com/doctrine/mongodb/blob/a226fce76171d9b22bfff05a9c32955022e46b2a/lib/Doctrine/MongoDB/Connection.php#L283

@moderndeveloperllc
Copy link
Contributor

@jmikola I had seen that in the test class, good to know that's the standard way. The logger is fine. You can pass save options, but there are no defaults.

As far as the 1.2 branch, 1.3.0 went final on PECL in Nov. 2012 and PHP 5.3.23 (current min version) came out March 2013. However < PHP 5.4.9 might have a <1.3.0 driver, so I think ZF2 will have to keep supporting < 1.3.0 until 5.4.9 is the minimum. Yes there is not a one to one correlation, but if you haven't updated PHP, it doubtful you've updated a specific extension. jm2c

@Nainterceptor
Copy link
Contributor Author

@jmikola True, I think this default parameter is useless.

MongoClient::getWriteConcern() return, by default, w => 1 and wtimeout => 500
Where can I found the old doc ? (< 1.3) to read around this behavior ?

Other way is phpversion('mongo') or following to deal with versions.

    $ext = new ReflectionExtension('mongo');
    $ext->getVersion();

@jmikola
Copy link
Contributor

jmikola commented May 15, 2014

There's no need to construct a ReflectionExtension instance when phpversion() works. There is also a MongoClient::VERSION constant, but phpversion() is the preferred method.

MongoClient::getWriteConcern() was only added in 1.5 of the driver, so you can't depend on that. It also doesn't necessarily return the default -- rather, it returns whatever MongoClient's write concern happens to be set to. By default, it is {w:1}, but you could just as easily change that at runtime or when constructing the client object.

I don't see any correlation between when versions of PHP and the MongoDB driver were released. The driver is not bundled with PHP as a core extension; instead, it's installed through PECL (a one-minute compilation). http://pecl.php.net/package/mongo also provides compiled binaries for Windows, so there is really no reason for someone to be on 1.2.x at this point. If they are, I do doubt they're using ZF2 and even less likely the session or log adapters. I'd defer to @Ocramius to make that decision, though.

@Nainterceptor
Copy link
Contributor Author

I talk to you about getWriteConcern only to attract your attention to the fact that this param is useless for recent MongoClient.
Probably, in old Mongo driver, we have default values for w or safe. In this case, it's maybe useless for zf2 to define same default values.

I think it's the simplest solution, like your previous link (Zend/Log/Writer)

gianarb pushed a commit to zendframework/zend-session that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-session that referenced this pull request May 15, 2015
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