colinmollenhour / mongodb-php-odm

A simple but powerful set of wrappers for using MongoDb in PHP (also a Kohana 3 module)
BSD 3-Clause "New" or "Revised" License
208 stars 50 forks source link

Mongo_Database::$configs got replaced in one of the pull requests #58

Closed panrafal closed 10 years ago

panrafal commented 10 years ago

Mongo_Database::$configs was there to provide external static configuration - for those who don't use it in Kohana. This commit 062d6c16e3e2612ce15f7339bf55e85002f038fd made it a protected property - essentially disabling this feature.

@sergeyklay, could you comment why you did this?

colinmollenhour commented 10 years ago

You can still use this without Kohana, you simply pass the config on the first use:

Mongo_Database::instance(NULL, $config);
...
Mongo_Database::instance()->authenticate(...);
panrafal commented 10 years ago

True, but you need to create an instance first, you can't make it lazy - with Kohana you can. But first and foremost, it was in my accepted PR. And now another PR changes it into something that does not make much sense - it stores current db's config in a protected variable, but first tries to read it!

colinmollenhour commented 10 years ago

The current code has the following properties:

Your patch changes this:

I think the current code is good and in line with conventions used in lots of other libraries. For example, I would guess that PDO does not allow you to read the database password after the connection is established. Also, your patch breaks backwards compatibility with 100% of non-Kohana usages since instance() is expected to set the config when called with a second parameter.

I do agree that is should be static, but I don't really agree that it should be public. Can you give an example scenario where using instance() to set the config is not sufficient and it would be solved by making configs public?

colinmollenhour commented 10 years ago

Ahh, I see what you are saying, the config should be set without instantiating an object. The connection itself is still lazy though so I don't see that instantiating the object is a big deal..

Regarding public/protected.. Imagine someone writes some terrible code that uses eval() and then a user could get the database password by something like print_r(Mongo_Database::$configs).

So really I think there doesn't need to be a "configs" property at all..

panrafal commented 10 years ago

Ok, it makes sense if the connection is lazy... In this case I agree we could remove the configs entirely. I will check if it works as it should in my setup and update this PR.

panrafal commented 10 years ago

I have removed $configs entirely, you can merge now...

sergeyklay commented 10 years ago

Initially I wanted order to has been closed configuration of direct access. At least the authentication information should not be in direct access. All Mongo_Database configuration can be saved in instance for further, secure work with it. There is nothing difficult to create an array configuration and pass it to the class constructor.

sergeyklay commented 10 years ago

I think for access to the some configuration data we need develop separate readers, instead, to make all configuration externally accessible.

colinmollenhour commented 10 years ago

Thanks for the input guys.

Sergey, I think the configuration access has extremely limited purpose so I propose that until someone actually comes up with a legitimate need for it, it should not be implemented.