{"issue":{"created_at":"2011-11-16T16:12:33Z","state":"open","updated_at":"2011-11-19T11:22:38Z","user":{"gravatar_id":"8ad62a25686796c42fedbe3ddedce1f1","id":99944,"url":"https://api.github.com/users/fzaninotto","login":"fzaninotto","avatar_url":"https://secure.gravatar.com/avatar/8ad62a25686796c42fedbe3ddedce1f1?d=https://a248.e.akamai.net/assets.github.com%2Fimages%2Fgravatars%2Fgravatar-140.png"},"comments":6,"body":"The `Propel` class is not a true singleton. It's a class full of static methods, not easily replaceable. I suggest to refactor it to a true [singleton class](http://en.wikipedia.org/wiki/Singleton_pattern), the only one in `Runtime`, holding a reference to all the necessary configuration:\r\n\r\n * connections\r\n * runtime model (Map classes)\r\n * adapters\r\n * logger\r\n\r\nI have started refactoring the `Propel` class to make the domain more apparent (see https://github.com/fzaninotto/Propel2/commit/2f1cbfb84d2264796aa5ade67c765156c11a61e0). This must go further.\r\n\r\nSo in the runtime code, any call to \r\n\r\n```\r\n$con = Propel::getConnection(BookPeer::DATABASE_NAME, Propel::CONNECTION_READ);\r\n```\r\n\r\nshould be rewritten to something like:\r\n\r\n```\r\n$con = Propel::getInstance()->getSlaveConnection(BookPeer::DATABASE_NAME);\r\n```\r\n\r\nThe naming is important. `Propel`might not be the best name. `Registry` also sounds too vague for me. It's a registry for runtime configuration - there are other types of registry in Propel. `Manager` is not good either, as it doesn't manage much more than configuration. Any suggestion?  I'd vote for `Propel\\Runtime\\Configuration`:\r\n\r\n```\r\n$con = Configuration::getInstance()->getSlaveConnection(BookPeer::DATABASE_NAME);\r\n```\r\n\r\nwhich, incidentally, is a little shorter than the `Propel::getConnection()` version.\r\n\r\nBut there is also a `Configuration` namespace in Runtime. However, it contains code that can be removed if the `convert-conf` class creates a `Configuration` singleton. Something like:\r\n\r\n```php\r\n<?php\r\n// in generated $bookstore-conf.php\r\n$conf = \\Propel\\Runtime\\Configuration::getInstance();\r\n$conf->addDatasource('bookstore', array(\r\n  'dsn' => 'mysql:dbname=test',\r\n  'user' => 'foo',\r\n  'password' => 'bar', \r\n  'options' => array('ATTR_PERSISTENT' => false)\r\n));\r\n$conf->addAdapter('bookstore', 'Propel\\Runtime\\Adapter\\MysqlAdapter');\r\n$conf->setDefaultDatasource('bookstore');\r\n```\r\n\r\nAdvantage: Configuration can easily be done by hand, using this class. The `convert-conf` task would just be a utility, taking XML as input but also YAML, or whatever you want. And in the Symfony2 integration, no need for that - the DI would populate the conf at will, maybe by subclassing it and injecting the DIC. That means we can probably get rid of the classes currently under `Propel\\Runtime\\Configuration`.\r\n\r\nI know it's a BC break, but I think it's necessary.\r\n\r\nComments?","assignee":null,"id":2258128,"number":51,"labels":[{"name":"RFC","color":"0b02e1","url":"https://api.github.com/repos/propelorm/Propel2/labels/RFC"},{"name":"Runtime","color":"e102d8","url":"https://api.github.com/repos/propelorm/Propel2/labels/Runtime"}],"pull_request":{"patch_url":null,"html_url":null,"diff_url":null},"title":"[RFC] Making a true Runtime Singleton","closed_at":null,"html_url":"https://github.com/propelorm/Propel2/issues/51","milestone":null,"url":"https://api.github.com/repos/propelorm/Propel2/issues/51"},"action":"created","comment":{"created_at":"2011-11-19T11:22:38Z","updated_at":"2011-11-19T11:22:38Z","user":{"gravatar_id":"8ad62a25686796c42fedbe3ddedce1f1","id":99944,"url":"https://api.github.com/users/fzaninotto","login":"fzaninotto","avatar_url":"https://secure.gravatar.com/avatar/8ad62a25686796c42fedbe3ddedce1f1?d=https://a248.e.akamai.net/assets.github.com%2Fimages%2Fgravatars%2Fgravatar-140.png"},"body":"And while we are at it... `getInstance()` is the sign of the Singleton pattern, which returns always the same instance of *the current* class. In this case, it's not a singleton, since it returns always the same instance of *another* class. To avoid confusion, I suggest to replace `Propel::getInstance()` by something like `Propel::getServices()`, or `Propel::getConfiguration()`, or whatever feels intuitive and short to write.","id":2798372,"url":"https://api.github.com/repos/propelorm/Propel2/issues/comments/2798372"}}