NotionCommotion Posted December 27, 2014 Share Posted December 27, 2014 I have a configuration file, and I use parse_ini_file() to parse it into an array at the initial entry point of my script. I would like the settings to be available to all downstream scripts/methods/functions. Note that my intent is not to change them outside of the configuration file, and ideally they will be readonly, however, if not, I suppose that is okay and I will just be sure not to modify them. I've read about dependency injection, and while this mostly makes sense, it seems like a lot of script/troubleshooting for little if any value for this scenario. I've read that global variables are bad (couples your objects to some "god" object, cannot unit test, difficult to troubleshoot, difficult to work with multiple developers, potential conflicts with other classes, namespace pollution), but maybe not for this case? Another option is defining a bunch of constants to the appropriate values, but this seems redundant. Or maybe some static class with all the values? What would be the best way to make configuration settings available to all PHP scripts, functions, and methods? Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted December 27, 2014 Share Posted December 27, 2014 This depends entirely on the architecture of your application. Is it mostly procedural code? Then simply make a get_setting() function which returns the value for a particular configuration key. Is it OOP with a central entry point like a front controller? Then instantiate a Configuration object in that main script and pass it to the request handlers (through the constructor, for example). If there's no central entry point, you either have to manually pass the object around, or you'll need a hard-coded reference to some Configuration class which has static methods for getting a configuration value. Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 27, 2014 Author Share Posted December 27, 2014 (edited) This depends entirely on the architecture of your application. Is it mostly procedural code? Then simply make a get_setting() function which returns the value for a particular configuration key. Is it OOP with a central entry point like a front controller? Then instantiate a Configuration object in that main script and pass it to the request handlers (through the constructor, for example). If there's no central entry point, you either have to manually pass the object around, or you'll need a hard-coded reference to some Configuration class which has static methods for getting a configuration value. Not mostly procedure code. I follow my self styled MVC architecture (probably sucks, but it seems to work for me. My concepts originated from "yikes" Joomla, but has vastly diverted). My initial script (index.php with a couple of classes to assist) is rather procedure based, and figures out which controller to use and invokes it. Controller does some logic, gets some data from the model, and passes it to a Twig template (thanks by the way). I also have some general library type classes, but suppose I could pass the configuration settings to them as needed. So, really just the controller classes and model classes need access to it (the Twig views and general library classes will be passed the data when needed). So one option is pass the settings to the controller, and have it in turn pass them to the model via constructors? Would the settings then be assessed as $this->Configuration->bla in both the controllers and models? What would using hard-coded reference to some Configuration class which has static methods for getting a configuration value look like? Edited December 27, 2014 by NotionCommotion Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted December 27, 2014 Share Posted December 27, 2014 So one option is pass the settings to the controller, and have it in turn pass them to the model via constructors? Would the settings then be assessed as $this->Configuration->bla in both the controllers and models? Yes. For convenience, make a base class with a getSetting() method which returns $this->configuration->getSetting('foo'). Then it's simply $this->getSetting('foo'). What would using hard-coded reference to some Configuration class which has static methods for getting a configuration value look like? Well, there would be one Configuration class which has a static getSetting() method. Whenever you need a setting in one of your classes, you call Configuration::getSetting('foo'). The downside is that you'll have a hard-coded reference to this specific class (see the discussion about dependency injection). Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 28, 2014 Author Share Posted December 28, 2014 The downside is that you'll have a hard-coded reference to this specific class (see the discussion about dependency injection). You mean this discussion: http://forums.phpfreaks.com/topic/292592-alternatives-to-globals/?hl=%2Bdependency+%2Binjection&do=findComment&comment=1497323? Please elaborate on the downsides of having to hard-coded reference to this specific class. I get why one shouldn't have some global variable where a bunch of methods read and modify at will, and that is not my intention, but only to have access to various configuration "constants". Quote Link to comment Share on other sites More sharing options...
kicken Posted December 28, 2014 Share Posted December 28, 2014 Having a hard-coded reference to your configuration class isn't really any different than just sticking global $configuration at the top of each function that you need to access the configuration from. If you ever try and do some unit testing, you have to create a Configuration class to satisfy those references, rather than just inject some other mock class for example. If you ever port the code to a different project that uses a different configuration setup you'll probably have to find those references and change them. Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 28, 2014 Author Share Posted December 28, 2014 Having a hard-coded reference to your configuration class isn't really any different than just sticking global $configuration at the top of each function that you need to access the configuration from. I agree. But is defining a bunch of constants (which I currently do) any better? If you ever try and do some unit testing, you have to create a Configuration class to satisfy those references, rather than just inject some other mock class for example. If you ever port the code to a different project that uses a different configuration setup you'll probably have to find those references and change them. Okay, but with all due respect, so what? Granted, I've never attempted to do unit testing, and am likely off base. But I just don't understand how this is different than defining a constant. Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted December 28, 2014 Share Posted December 28, 2014 A bunch of constants is even less modular, because now you've hard-coded a specific configuration mechanism, not just a certain class. If you later decide to, for example, put the configuration into an external JSON file, it can't be done unless you either rewrite all classes or implement some kind of emulation layer which defines the necessary constant at runtime (not pretty). For clarification, none of those approaches is inherently wrong. You'll see all of them in reputable projects, and they work fine for the specific case. If I just write a bunch of procedural scripts from scratch, I don't care about modularity or code recuse. I simply stuff all configuration values into constants. At our company, we also stick to procedural code, and the default configuration is stored in a $CONFIG global which we can override at runtime (with a local development configuration, for example). Your case is different, though: You have an OOP framework. Modularity makes a lot of sense here, because the configuration mechanism may change. You might use different mechanisms for different projects, or maybe some day you decide that you want a clean YAML file instead of all those constants. Why not prepare for that? Your framework should accept any configuration logic. Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 28, 2014 Author Share Posted December 28, 2014 A bunch of constants is even less modular, because now you've hard-coded a specific configuration mechanism, not just a certain class.... For clarification, none of those approaches is inherently wrong. I agree all these constants are even less modular, and I feel my approach is somewhat inherently wrong. I have defined about 50 constants, and use them over 1,000 locations in a given application, and it is rather overwhelming. Let me explain where I use them. Defining filesystem paths such as the location of my classes, etc. I suppose this is okay. Defining configuration constants which are obtained from my previously mentioned parsed configuration file. This seems like a waste. Defining stuff about the request such as the protocol or requested content. This is probably not ideal. Defining two special constants which identify the account and the user. This is probably not right, but it seems to make life easier. Your case is different, though: You have an OOP framework. Modularity makes a lot of sense here, because the configuration mechanism may change. You might use different mechanisms for different projects, or maybe some day you decide that you want a clean YAML file instead of all those constants. Why not prepare for that? Your framework should accept any configuration logic. How wold you recommend changing it? <?php //Define file locations define( 'DS', DIRECTORY_SEPARATOR ); define('USR_CLASSES',__DIR__); define('USR_APPLICATION_BASE',dirname(USR_CLASSES)); define('USR_ROOT_BASE',dirname(USR_APPLICATION_BASE)); define('USR_TWIG',USR_ROOT_BASE.'/vendor/autoload.php'); define('USR_LIB',USR_APPLICATION_BASE.DS.'lib'); define('USR_HELP',USR_LIB.DS.'help'); define('USR_TEMPLATES',USR_LIB.DS.'templates'); define('USR_HTDOCS_BASE',USR_ROOT_BASE.DS.'html'); define('USR_RESOURCES_BASE',USR_ROOT_BASE.DS.'user_resources'); define('USR_DOCUMENTS_USER', USR_RESOURCES_BASE.DS.'documents'); define('USR_DOCUMENTS_TMP', USR_RESOURCES_BASE.DS.'tmp'); define('USR_DOCUMENTS_TMPSAVE', USR_RESOURCES_BASE.DS.'tmpsave'); define('USR_OS_TICKET',USR_HTDOCS_BASE.DS.'support'); //Set configuration constants $this->init = parse_ini_file(USR_APPLICATION_BASE."/config.ini"); define('USR_PASSWORD_ALGORITHM',$this->init['PASSWORD_ALGORITHM']); define('USR_PASSWORD_COST',$this->init['PASSWORD_COST']); define('USR_GOOGLE_MAP_KEY',$this->init['GOOGLE_MAP_KEY']); define('USR_IPINFODB_KEY',$this->init['IPINFODB_KEY']); define('USR_RECAPTCHA_PUBLIC', $this->init['RECAPTCHA_PUBLIC'] ); define('USR_RECAPTCHA_PRIVATE', $this->init['RECAPTCHA_PRIVATE'] ); define('USR_MEMORY_LIMIT',$this->init['MEMORY_LIMIT']); define('USR_MAX_FILE_SIZE_BACK',$this->init['MAX_FILE_SIZE_BACK']); define('USR_MAX_FILE_SIZE_FRONT',$this->init['MAX_FILE_SIZE_FRONT']); define('USR_MAX_FILE_SIZE_UPLOAD_RECORDS',$this->init['MAX_FILE_SIZE_UPLOAD_RECORDS']); define('USR_PUBLIC_ACCESS_ID',$this->init['PUBLIC_ACCESS_ID']); define('USR_USER_ACCESS_ID',$this->init['USER_ACCESS_ID']); define('USR_ADMIN_ACCESS_ID',$this->init['ADMIN_ACCESS_ID']); define('USR_TECHSUPPORT_ACCESS_ID',$this->init['TECHSUPPORT_ACCESS_ID']); define('USR_ROOT_ACCESS_ID',$this->init['ROOT_ACCESS_ID']); define('USR_PRIVATE_SALT',$this->init['PRIVATE_SALT']); define('USR_RANKING_CONSTANT',$this->init['RANKING_CONSTANT']); define('USR_COMPONENTS_BASE',USR_APPLICATION_BASE.DS.'components'.DS.'back'); define('USR_MAX_TOTAL_LOGON_ATTEMPTS',$this->init['MAX_TOTAL_LOGON_ATTEMPTS_BACK']); define('USR_LOGONS_HOURS_IP_BAN',$this->init['LOGONS_HOURS_IP_BAN_BACK']); define('USR_MAX_IP_LOGON_ATTEMPTS',$this->init['MAX_IP_LOGON_ATTEMPTS_BACK']); define('USR_LOGONS_HOURS_BAN',$this->init['LOGONS_HOURS_BAN_BACK']); define('USR_MAX_LOGON_ATTEMPTS',$this->init['MAX_LOGON_ATTEMPTS_BACK']); //Define stuff about the request define('USR_PROTOCOL',( (isset($_SERVER['HTTPS']) && strtolower($_SERVER['HTTPS']) == 'on') || (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && strtolower($_SERVER['HTTP_X_FORWARDED_PROTO']) == 'https') || $_SERVER['SERVER_PORT'] == 443 )?'https':'http'); define('USR_PRIMARY_DOMAIN', $domain[count($domain)-2].'.'.$domain[count($domain)-1]); define('USR_URI', USR_PROTOCOL.'://'.$_SERVER['HTTP_HOST']); define('USR_CSRF',isset($_SESSION['CSRF'])?$_SESSION['CSRF']:null); define('USR_TIMESTAMP',time()); define('USR_TIMESTAMP_OLD',isset($_SESSION['TIMESTAMP'])?$_SESSION['TIMESTAMP']:null); define('USR_JAVASCRIPT_ENABLED',isset($_SESSION['TIMESTAMP']) && isset($_COOKIE['TIMESTAMP']) && $_SESSION['TIMESTAMP']==$_COOKIE['TIMESTAMP']); define('USR_CONTENT_REQUEST',isset($_GET['request_type'])?$_GET['request_type']:(isset($_POST['request_type'])?$_POST['request_type']:'text')); define('USR_IS_ASYNC',isset($_POST['is_async'])?$_POST['is_async']:(isset($_GET['is_async'])?$_GET['is_async']:(isset($_SERVER['HTTP_X_REQUESTED_WITH'])?$_SERVER['HTTP_X_REQUESTED_WITH']:false))); //Define special two constants define('USR_ACCOUNT_ID',$this->accountData->id); define('USR_USERS_ID',$this->userData->id); ?> Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted December 28, 2014 Share Posted December 28, 2014 This is indeed way too much. The script has become a kind of data dump where you put everything that is somehow relevant for you. Keep things separate: First of all, there's the app configuration. The data should be in a separate file for easy access (INI is fine), and you should process it as described above. So you have a Configuration object with a method like getSetting(). Then there's the meta data about the request. Consider making a Request object with methods like isHTTPS(), isJavaScriptEnabled() etc. And finally, you have a fixed set of data about your application (paths etc.). Why not make an App object with methods like getLibPath()? In addition to that, you may want specialized classes, for example a Template class for managing all the Twig stuff. This is really what OOP is all about: You divide your application into specific aspects, and then you design objects for those aspects. Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 28, 2014 Author Share Posted December 28, 2014 Thanks Jacques1, I appreciate and agree with your advice. Since the data is static, would you recommend a static class, or a traditional class where the objects are passed to those classes that need them? Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 28, 2014 Author Share Posted December 28, 2014 For instance, something like the following: <?php class Configuration { private static $instance = NULL; private function __construct() {} //Make private private function __clone(){} //Make private public static function getSetting($prop) { if (!self::$instance) { $config = parse_ini_file("../../application/config.ini"); self::$instance=new stdClass(); foreach($config as $key=>$value) { self::$instance->$key=$value; } } return self::$instance->$prop; } } class Request { public static function isJavaScriptEnabled() { return isset($_SESSION['TIMESTAMP']) && isset($_COOKIE['TIMESTAMP']) && $_SESSION['TIMESTAMP']==$_COOKIE['TIMESTAMP']; } public static function isHTTPS() { return ( (isset($_SERVER['HTTPS']) && strtolower($_SERVER['HTTPS']) == 'on') || (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && strtolower($_SERVER['HTTP_X_FORWARDED_PROTO']) == 'https') || $_SERVER['SERVER_PORT'] == 443 ); } public static function isAsync() { return isset($_POST['is_async'])?$_POST['is_async']:(isset($_GET['is_async'])?$_GET['is_async']:(isset($_SERVER['HTTP_X_REQUESTED_WITH'])?$_SERVER['HTTP_X_REQUESTED_WITH']:false)); } public static function getRequestedContent() { return isset($_GET['request_type'])?$_GET['request_type']:(isset($_POST['request_type'])?$_POST['request_type']:'text'); } public static function getPrimaryDomain($domain) { return $domain[count($domain)-2].'.'.$domain[count($domain)-1]; } public static function getURI() { return (self::isHTTPS?'https':'http').'://'.$_SERVER['HTTP_HOST']; } public static function getCSRF() { return isset($_SESSION['CSRF'])?$_SESSION['CSRF']:null; } public static function getPreviousTimestamp() { return isset($_SESSION['TIMESTAMP'])?$_SESSION['TIMESTAMP']:null; } public static function getTimestamp() { //Get rid of this one, and use $_SERVER['REQUEST_TIME'] return time(); } } class App { public static function getLibPath() { //return library path, etc } } //will parse file echo(Configuration::getSetting('TIMEZONE').'<br>'); //will not parse file echo(Configuration::getSetting('LOCALITY').'<br>'); ?> Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 28, 2014 Author Share Posted December 28, 2014 Ended up changing the Configuration class to support multiple levels. class Configuration { private static $instance = NULL; private function __construct() {} //Make private private function __clone(){} //Make private private static function init() { $config = parse_ini_file("../../application/config.ini",true); self::$instance=$config; } public static function getOther($prop) { if (!self::$instance){self::init();} return self::$instance['other'][$prop]; } public static function getAccess($prop) { if (!self::$instance){self::init();} return self::$instance['access'][$prop]; } } Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted December 28, 2014 Share Posted December 28, 2014 I find the implementation a bit weird. The Configuration class pretends to be a singleton with the instance stored in $instance, but then it turns out that $instance is actually an array from parse_ini_file(). This makes no sense. You're also going through a lot of trouble only to have static methods when that's actually the poorer solution compared to a standard object with dependency injection. Why not simply instantiate a Configuration object, do the config file parsing in the constructor and then pass the object to whoever needs it? It doesn't even need to be a singleton. If you pass the file path to the constructor, you can have multiple different configurations, and you avoid the hard-coded path in the class: <?php class Config { protected $settings; public function __construct(App $app, $config_file) { // make sure the config file is valid (just an example) if (preg_match('/\\A[a-z_][a-z0-9_]*\\.ini\\z/i', $config_file)) { $settings = parse_ini_file($app->getConfigDir() . $config_file, true); if ($settings !== false) { $this->settings = $settings; } else { throw new Exception('Failed to parse configuration file'); } } else { throw new Exception('Invalid configuration file.'); } } } And then you use it like this: $base_config = new Config($app, 'base.ini'); $password_algorithm = $base_config->getSetting('security.password_algorithm'); Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 29, 2014 Author Share Posted December 29, 2014 Oh oh, not a bit weird I like the use of JavaScript object notation which is a little less verbose (i.e. security.password_algorithm). I see you are passing presumably the application ($app)? to the class, and then type hinting it as App. I usually think of the application as $this. Are you passing the parent controller or something into the config class so you could use the getConfigDir() method? Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted December 29, 2014 Share Posted December 29, 2014 I see you are passing presumably the application ($app)? to the class, and then type hinting it as App. I usually think of the application as $this. Are you passing the parent controller or something into the config class so you could use the getConfigDir() method? No, $app is literally the App object. Since the configuration clearly belongs to the application, it makes sense to create it within the App class and expose it through a getSetting() method. Then you don't need an extra parameter in the controllers. To get a setting, you simply call $app->getSetting('security.password_hash_algorithm') which then delegates the task to its Config object. Quote Link to comment Share on other sites More sharing options...
ignace Posted December 29, 2014 Share Posted December 29, 2014 (edited) Currently your Configuration object doesn't allow many different uses. Using the full extend of OOP, you get something like this: interface Configuration { function get($key, $default = null); function set($key, $value); } class ArrayConfiguration implements Configuration { private $data; public function __construct(array $data) { $this->data = $data; } // implement methods } class IniConfiguration extends ArrayConfiguration { function __construct(SplFileInfo $filepath, $processSections = false, $scannerMode = INI_SCANNER_NORMAL) { parent::__construct(parse_ini_file($filepath, $processSections, $scannerMode)); } }Now you can create Configuration from an array or from INI files. This logic can also be put into an object: class ConfigurationFactory { public function createFromSource($source) { if (is_array($source)) { return new ArrayConfiguration($source); } elseif (is_file($source) && '.ini' === substr($source, -4)) { return new IniConfiguration($source); } else { throw new RuntimeException('Type of source is not supported, expected array or filepath got ' . gettype($source)); } } } $factory = new ConfigurationFactory(); $factory->createFromSource(array('hello' => 'world')); // returns ArrayConfiguration $factory->createFromSource('/path/to/file.ini'); // returns IniConfiguration function doSomethingBasedOnConfigValue(Configuration $cfg) { if ($cfg->get('foo') == 'bar') { // do bar } }The above code is unaware if this value came from a simple array (during testing) or from a ini source file (in production). Even better now you can extend your code without altering your existing code. class ApcConfiguration extends ArrayConfiguration { function __construct($cacheKey) { parent::__construct(apc_fetch($cacheKey)); } }APC stores data in memory and is persisted (shared) between requests avoiding to load from the hard drive (which is way slower then simply loading from memory). doSomethingBasedOnConfigValue(new ApcConfiguration('my_cache_key'));As you see the above code does not have to change although your configuration is now loaded from a shared source. Edited December 29, 2014 by ignace Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted December 29, 2014 Share Posted December 29, 2014 ignace doing his favorite thing, overengineering. What you totally forgot, though, is the JavaSeriousBusinessEnterpriseNoSQLCloudConfigurationFactoryObserver! Now the code is not hip anymore. Quote Link to comment Share on other sites More sharing options...
ignace Posted December 30, 2014 Share Posted December 30, 2014 (edited) It's not over-engineered instead it's practical. I believe having at least one interface for a class that might change in the future is a good thing and many will agree with me as is making the OP aware of this. Always keep an open-mind that the OP could be writing a program that at some point could get lots of load. Teach a man to fish etc.. If you have a legitimate reason why my code is bad practice then please do note them, if not keep your childish comments for yourself. Also I insist you refrain from hijacking the OP's topic further. Edited December 30, 2014 by ignace Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted December 30, 2014 Share Posted December 30, 2014 Buddy, it's actually you who permanently hijacks discussions with his “I'd-rather-be-a-Java-programmer-but-they-only-let-me-do-PHP” overengineering nonsense. When a user just wants to do a simple SQL query, you tell them to set up an ORMAbstractQueryBuilderInterfaceProvider, because SQL is sooo 2000, and they totally need to support MongoDB and CouchDB and Redis and who-knows-what. If they just want a simple configuration for their scripts, you tell them to use an APCFactoryConfigurationObserver, because an INI file just ain't enterprisey enough. We totally need APC. And Elasticsearch. And of course lots of XML. What's the matter? You seem to spend your entire time in that OOP hipster ivory tower, turning even the most simple problem into a pattern massacre with 500 interfaces. Do you know “FizzBuzzEnterpriseEdition”? That's what I'm talking about. Yes, there are tons of fancy design patterns, and I applaud you for knowing all of them. But we're not here to show off our mad OOP skills. We're here to solve real problems in the real world. Just think about it: NotionCommotion currently has a simple INI file, and this is perfectly legitimate. In a couple of years, he might want to replace that with a different format like, say, JSON. That's it. One configuration class, maybe one interface (as I already suggested), and we're done. If you tell him to set up a ConfigurationFactory so that he can have APC and INI and arrays all at the same time, you're entirely missing the point. Don't get me wrong: I admire you for your theoretical knowledge. If I had a question about some design pattern or the latest Java trends, I'd surely ask you. But that's not the topic of this thread. If you want to talk about something else, you should create your own thread. Quote Link to comment Share on other sites More sharing options...
ignace Posted December 30, 2014 Share Posted December 30, 2014 (edited) Ok yeah sure I hammer everything with OOP but that does not mean you have to ridicul it to such extremes because my solutions are spot-on. And sure I never keep it to simply the solution and extend further upon it to show the OP what other options it extends him with. Because it might just be that at some point he considers to cache his configuration with APC which is far better then loading it from INI files on each request. And I am not showing off there are people here that are far better at it then I am. And at the very least I try to learn the OP some new things not bitch about other people's posts, how contributing of you. And I DO solve real problems. 70,000 requests per second problems I solve every day, keeping the number of servers to an absolute minimum for which many even applaud my solutions. So I may be in an ivory, but so are you thinking your solutions and intellect solve world-hunger. Edited December 30, 2014 by ignace Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted December 30, 2014 Author Share Posted December 30, 2014 I very much appreciate both your help, as well as that from many others on this forum. I think back at where I started and where I am now, and while not yet perfect, have made mountainous strides. Please feel free to consider this post closed. Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted December 30, 2014 Share Posted December 30, 2014 @ignace: I do not question your skills and your experience, and I've gone overboard with the previous reply. I'm sorry. However, I do think that simplicity is very important, more important than perfect extensibility. If NotionCommotion actually decides to use APC somewhere in the future, he can simply replace the Configuration class (while keeping the interface). That's only a matter of a few minutes, it won't happen too often, and it makes the code so much easier. Keeping all kinds of different implementations in a factory seems overkill. But like you said, the decision is always up to the OP. Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.