The following code works and does what I want, but I'd like to confirm I'm using this OOP concept correctly. I'm using the following class to get and set some configuration parameters for my program. It used $GLOBALS for this before, but I'm trying to use a singleton pattern for this instead.
Below is the Config class. Does anything jump out as aggressively stupid? Seeing as how I'm going to be including this in most files any way I figured it would make sense to throw the autoload function in there too- is this bad design?
<?php
//This class will be included for every file
//needing access to global settings and or
//the class library
spl_autoload_register(function($class){
require_once 'classes/' . $class . '.php';
});
final class Config {
private static $inst = null;
//Array to hold global settings
private static $config = array(
'mysql' => array(
'host' => '127.0.0.1',
'username' => 'root',
'password' => '123456',
'db' => NULL
),
'shell' => array(
'exe' => 'powershell.exe',
'args' => array(
'-NonInteractive',
'-NoProfile',
'-NoLogo',
'-Command'
)
)
);
public static function getInstance() {
if (static::$inst === null) {
static::$inst = new Config();
}
return static::$inst;
}
public function get($path=NULL) {
if($path) {
//parse path to return config
$path = explode('/', $path);
foreach($path as $element) {
if(isset(static::$config[$element])) {
static::$config = static::$config[$element];
} else {
//If specified path not exist
static::$config = false;
}
}
}
//return all configs if path NULL
return static::$config;
}
public function set($path=NULL,$value=NULL) {
if($path) {
//parse path to return config
$path = explode('/', $path);
//Modify global settings
$setting =& static::$config;
foreach($path as $element) {
$setting =& $setting[$element];
}
$setting = $value;
}
}
//Override to prevent duplicate instance
private function __construct() {}
private function __clone() {}
private function __wakeup() {}
}
I access it like so:
$aaa = Config::getInstance();
var_dump($aaa->get());
$aaa->set('mysql/host','zzzzzzzzzzzz');
var_dump($aaa->get());
I read a little bit about how singletons are less useful in php. I will research that further at some point, but for the time being I just want to make sure I'm doing this right.
2 Answers 2
There should be something about a Singleton class that requires that it allow and present a single, globally available, instance. Otherwise, the pattern is being misused.
The way your class is built, there's not really such a reason. You could have any number of instances, and they'd all work (though they'd all look like the same instance anyway).
As for recommendations to make things less weird...?
Use instance variables.
Everything is essentially global. (Make no mistake; hiding it in a class doesn't do much at all to change that.
static
is just how a lot of OO languages spell "global". :P) Your "instance" is just an interface to get and set that global stuff. Every "instance" has the same ability, since it gets and sets the same variables -- and in fact, any update through one "instance" will be reflected in every other. Sonew Config
andConfig::getInstance()
really differ only in whether another instance is created. The caller could say one or the other, and get the exact same results.If you insist on a singleton, moving the
static
variables into the instance would legitimize the existence of an instance at all, and provide a less dodgy reason to require exactly one authoritative instance. (Though it'd also prompt the question, "Why can't i create a separate configuration for testing?". Among others.)Lose the public setter, or provide a way to disable it.
There's next to no encapsulation or data hiding going on here; the only thing that even remotely counts is your path stuff. Any caller can change global settings at will. This kind of "action at a distance" can cause all kinds of weirdness. It is the very reason mutable global state is discouraged, and
set
makes it even easier to abuse.You should consider either getting rid of the setter, making it private (and thus disabling outside modification altogteher), or adding some way for your setup code to disable it -- effectively "freezing" the instance -- once the configuration is established.
Use
self::
, notstatic::
.static
(when used in place of a class name) uses PHP's "late static binding" to look up variables through the name of the called class rather than the one where the method lives. It's there to allow subclasses to override static members. But you've explicitly disallowed inheritance...so values will only ever be looked up in this class.static
is thus wasteful, and a bit of a mixed message.As for the autoloader, it's not exactly a mortal sin or anything to put that in the same file as the config class. Unless the classes are going to be moving around, though, their current location isn't really part of the config settings, is it?
I'd suggest making the autoloader a part of the startup code, unless the class files are going to move around (thus making the current location a part of the configuration).
-
\$\begingroup\$ Thanks for all the great info. I thought that static replaced self and that the behavior of self was a flaw not a feature. Is it wrong to just always use static or are there performance considerations? If declare a class final shouldn't that clear up any confusion? It would just be nice to only have one keyword for this instead of juggling two. Thanks again. \$\endgroup\$user1028270– user10282702014年01月17日 14:29:38 +00:00Commented Jan 17, 2014 at 14:29
-
2\$\begingroup\$ It can be wrong to use
static::
. If you have class B that extends class A, andA::method
usesstatic::$stuff
, thenB::method()
will first look forB::$stuff
, thenA::$stuff
. Sometimes you don't want that. In your current case, "wrong" is a bit of an overstatement. But the whole point ofstatic::
is to cause LSB, so it is really better used only when you (1) can have subclasses, and (2) want them to be able to override a static attribute. LSB implies extensibility, which contradicts the one line (more than a screen away from most uses of it) that outlaws extension. \$\endgroup\$cHao– cHao2014年01月17日 17:24:47 +00:00Commented Jan 17, 2014 at 17:24 -
\$\begingroup\$ The difference between
self
vsstatic
is important, and sometimes quite desirable -- particularly if you have some functionality you don't want overridden, or want to be certain of where you're setting a variable but don't want to hard-code the class name. \$\endgroup\$cHao– cHao2014年01月17日 17:38:49 +00:00Commented Jan 17, 2014 at 17:38 -
\$\begingroup\$ Thanks for the great explanation, that makes sense to me now. So self can be useful if I am extending a class, but also want to access a method in the parent instead of the local instance of that method. \$\endgroup\$user1028270– user10282702014年01月17日 17:46:43 +00:00Commented Jan 17, 2014 at 17:46
-
1\$\begingroup\$ @theking2 : The issue is not of correctness, but of clarity, and for reasons already mentioned. If you don't allow inheritance, then leaning on LSB is wasteful and confusing, as the whole point of LSB is to allow overriding of static stuff. \$\endgroup\$cHao– cHao2023年01月02日 15:22:58 +00:00Commented Jan 2, 2023 at 15:22
I just want to add to cHao's explanation of static
vs self
. You use self
to call a static function rather than do $this->
because you can only do $this
if you have instantiated an object.
This is typically done when you want to call a function that belongs to a class but that function has static variables (like a hard coded constant/variable) that is used within the function.
-
\$\begingroup\$ In the case of a singleton pattern there is one and just one instance of the class. In the sample of the OP this single class having static variables is not following this pattern. \$\endgroup\$theking2– theking22023年01月04日 07:31:31 +00:00Commented Jan 4, 2023 at 7:31
const arrray
. I don't see an advantage of changing settings in code. Would on the other hand would be helpful is reading settings from a file withparse_ini_file
. Store an ini file in the root of your project and read it withparse_ini_file( $_SERVER['DOCUMENT_ROOT'/app.conf')
Make sure theapp.conf
cannot be read by others \$\endgroup\$