4

I am creating a class which I will use to store and load some settings. Inside the class all settings are stored in an array. The settings can be nested, so the settings array is a multidimensional array. I want to store and load the settings using the magic methods __get and __set, so the settings can act as class members. However, since I'm using nested methods, I can't get the __set method to work when I try to access a nested setting.

The class is like this:

class settings
{
    private $_settings = array();

    //some functions to fill the array

    public function __set($name, $value)
    {
        echo 'inside the __set method';
        //do some stuff
    }
}

And the code to use this class:

$foo = new settings();
//do some stuff with the class, so the internal settings array is as followed:
//array(
//    somename => somevalue
//    bar => array (
//               baz = someothervalue
//               qux = 42
//                 )
//     )
$foo->somename = something; //this works, __set method is called correctly
$foo->bar['baz'] = somethingelse; //Doesn't work, __set method isn't called at all

How can I get this last line to work?

Tiddo
  • 6,331
  • 6
  • 52
  • 85

4 Answers4

8

When accessing an array using this method, it actually goes through __get instead. In order to set a parameter on that array that was returned it needs to be returned as a reference: &__get($name)

Unless, what you mean is that you want each item that is returned as an array to act the same way as the parent object, in which case you should take a look at Zend Framework's Zend_Config object source for a good way to do that. (It returns a new instance of itself with the sub-array as the parameter).

Zimzat
  • 654
  • 3
  • 14
  • It shouldn't act the same way as the parent object. Returning by reference is a nice idea, but than I can't return values in the usual way (e.g. $bar = $foo->somename; $bar = somethingelse would change the class itself) – Tiddo Sep 21 '11 at 13:23
  • 1
    @Tiddo: no it wouldn't. You'd need to do `$bar = &$foo->somename` for that to happen. You need the reference argument on both sides in order for the reference to be created. Otherwise it will just silently be dropped. Give it a try... – ircmaxell Sep 21 '11 at 13:35
  • You're right, I forgot the &. But than I need a different syntax for arrays and simple variables? It's not an ideal solution, but it's the best so far I think. – Tiddo Sep 21 '11 at 13:50
  • To prove that this is the correct answer, take a look at [this codepad](http://codepad.viper-7.com/1nl8NN). It does indeed work and doesn't set references all over the place... – ircmaxell Sep 21 '11 at 13:50
  • Precisely as @ircmaxell says. That's the beauty of the return by reference system (unlikely pass by reference). Both the returner and the returnee have to accept it as a reference, otherwise neither happens. – Zimzat Sep 21 '11 at 13:51
  • @Tiddo: If you're doing $settingsObject->abc['xyz'] = 123; then it's handled automatically for you. If you're doing $ref = $settingsObject->abc; $abc['xyz'] = 123;, then it's not, and you're misunderstanding the nature of arrays vs objects. – Zimzat Sep 21 '11 at 13:54
  • @zimzat: I know, recently I've been working more with C++ than php, and C++ handles references and arrays a little different than php, so I was mixing some things up. – Tiddo Sep 21 '11 at 14:00
5

This would work:

$settings = new Settings();
$settings->foo = 'foo';
$settings->bar = array('bar');

But, there is no point in using magic methods or the internal array at all. When you are allowing getting and setting of random members anyway, then you can just as well make them all public.

Edit after comments (not answer to question above)

Like I already said in the comments I think your design is flawed. Let's tackle this step by step and see if we can improve it. Here is what you said about the Settings class requirements:

  • settings can be saved to a file or a database
  • settings might need to update other parts of the application
  • settings need to be validated before they are changed
  • should use $setting->foo[subsetting] over $setting->data[foo[subsetting]]
  • settings class needs to give access to the settings data for other classes
  • first time an instance is made, the settings need to be loaded from a file

Now, that is quite a lot of things to do for a single class. Judging by the requirements you are trying to build a self-persisting Singleton Registry, which on a scale of 1 (bad) to 10 (apocalyptic) is a level 11 idea in my book.

According to the Single Responsibility Principle (the S in SOLID) a class should have one and only reason to change. If you look at your requirements you will notice that there is definitely more than one reason to change it. And if you look at GRASP you will notice that your class takes on more roles than it should.

In detail:

settings can be saved to a file or a database

That is at least two responsibilites: db access and file access. Some people might want to further distinguish between reading from file and saving to file. Let's ignore the DB part for now and just focus on file access and the simplest thing that could possibly work for now.

You already said that your settings array is just a dumb key/value store, which is pretty much what arrays in PHP are. Also, in PHP you can include arrays from a file when they are written like this:

<?php // settings.php
return array(
    'foo' => 'bar'
);

So, technically you dont need to do anything but

$settings = include 'settings.php';
echo $settings['foo']; // prints 'bar';

to load and use your Settings array from a file. This is so simple that it's barely worth writing an object for it, especially since you will only load those settings once in your bootstrap and distribute them to the classes that need them from there.

Saving an array as an includable file isnt difficult either thanks to var_export and file_put_contents. We can easily create a Service class for that, for example

class ArrayToFileService
{
    public function export($filePath, array $data)
    {
        file_put_contents($filePath, $this->getIncludableArrayString($data));
    }
    protected function getIncludableArrayString($data)
    {
        return sprintf('<?php return %s;', var_export($data, true));
    }
}

Note that I deliberatly did not make the methods static despite the class having no members of it's own to operate on. Usign the class statically will add coupling between the class and any consumer of that class and that is undesirable and unneccessary.

All you have to do now to save your settings is

$arrayToFileService = new ArrayToFileService;
$arrayToFileService->export('settings.php', $settings);

In fact, this is completely generic, so you can reuse it for any arrays you want to persist this way.

settings might need to update other parts of the application

I am not sure why you would need this. Given that our settings array can hold arbitrary data you cannot know in advance which parts of the application might need updating. Also, knowing how to update other parts of the application isnt the responsiblity of a data container. What we need is a mechanism that tells the various parts of the application when the array got updated. Of course, we cannot do that with a plain old array because its not an object. Fortunately, PHP allows us to access an object like an array by implementing ArrayAccess:

class HashMap implements ArrayAccess
{
    protected $data;

    public function __construct(array $initialData = array())
    {
        $this->data = $initialData;
    }
    public function offsetExists($offset)
    {
        return isset($this->data[$offset]);
    }
    public function offsetGet($offset)
    {
        return $this->data[$offset];
    }
    public function offsetSet($offset, $value)
    {
        $this->data[$offset] = $value;
    }
    public function offsetUnset($offset)
    {
        unset($this->data[$offset]);
    }
    public function getArrayCopy()
    {
        return $this->data;
    }
}

The methods starting with offset* are required by the interface. The method getArrayCopy is there so we can use it with our ArrayToFileService. We could also add the IteratorAggregate interface to have the object behave even more like an array but since that isnt a requirement right now, we dont need it. Now to allow for arbitrary updating, we add a Subject/Observer pattern by implementing SplSubject:

class ObservableHashMap implements ArrayAccess, SplSubject
…
    protected $observers;

    public function __construct(array $initialData = array())
    {
        $this->data = $initialData;
        $this->observers = new SplObjectStorage;
    }
    public function attach(SplObserver $observer)
    {
        $this->observers->attach($observer);        
    }
    public function detach(SplObserver $observer)
    {
        $this->observers->detach($observer);        
    }
    public function notify()
    {
        foreach ($this->observers as $observers) {
            $observers->update($this);
        }
    }
}

This allows us to register arbitrary objects implementing the SplObserver interface with the ObservableHashMap (renamed from HashMap) class and notify them about changes. It would be somewhat prettier to have the Observable part as a standalone class to be able to reuse it for other classes as well. For this, we could make the Observable part into a Decorator or a Trait. We could also decouple Subject and Observers further by adding an EventDispatcher to mediate between the two, but for now this should suffice.

Now to notify an observer, we have to modify all methods of the class that should trigger a notification, for instance

public function offsetSet($offset, $value)
{
    $this->data[$offset] = $value;
    $this->notify();
}

Whenever you call offsetSet() or use [] to modify a value in the HashMap, any registered observers will be notified and passed the entire HashMap instance. They can then inspect that instance to see whether something important changed and react as needed, e.g. let's assume SomeComponent

class SomeComponent implements SplObserver
{
    public function update(SplSubject $subject)
    {
        echo 'something changed';
    }
}

And then you just do

$data = include 'settings.php';
$settings = new ObservableHashMap($data);
$settings->attach(new SomeComponent);
$settings['foo'] = 'foobarbaz'; // will print 'something changed'

This way, your settings class needs no knowledge about what needs to happen when a value changes. You can keep it all where it belongs: in the observers.

settings need to be validated before they are changed

That one is easy. You dont do it inside the hashmap/settings object at all. Given that the HashMap is just a dumb container holding arbitrary data that is supposed to be used by other classes, you put the validation into those classes that use the data. Problem solved.

should use $setting->foo[subsetting] over $setting->data[foo[subsetting]]

Well, yeah. As you probably have guessed already, the above implementation doesnt use this notation. It uses $settings['foo'] = 'bar' and you cannot use $settings['foo']['bar'] with ArrayAccess (at least to my knowledge). So that is somewhat of a limitation.

settings class needs to give access to the settings data for other classes

This and the next requirement smell like Singleton to me. If so, think again. All you ever need is to instantiate the settings class once in your bootstrap. You are creating all the other classes that are required to fulfill the request there, so you can inject all the settings values right there. There is no need for the Settings class to be globally accessible. Create, inject, discard.

first time an instance is made, the settings need to be loaded from a file

See above.

Community
  • 1
  • 1
Gordon
  • 312,688
  • 75
  • 539
  • 559
  • The class is of course a little more complicated than the few lines in my question. However, that wasn't really relevant for my question. I can't just add the values to the class in the way you described (and I think that's a very ugly way to store values. That's not how OOP was meant to be) – Tiddo Sep 21 '11 at 13:12
  • @Tiddo Actually what you describe is how the values get stored to the class so if you think it's ugly, dont store them that way. If all those values are accessible anyway, you can just as well make them public. And besides: using `__set` for lazy setters isnt exactly good practise either, nor is having an Object with random members very much OOP. – Gordon Sep 21 '11 at 13:17
  • I think class member variables shouldn't be created from outside the class. And also I think you should avoid public member variables as much as possible. And finally(for the public part), I prefer the syntax $settingobject->somesetting[subsetting] over $settingobject->settingarray[somesetting[subsetting]]. The former one is much cleaner. Edit: I think the whole reason we write wrapper classes is to avoid the syntax you provided above. Correct me if I'm wrong, but I think most language don't even support, or at least discourage, the use of something like this. – Tiddo Sep 21 '11 at 13:26
  • @Tiddo I agree to that, but then why do you want the `__set` method to act like a setter for those members? That's contradicting. `$foo->bar = 'baz'` *is* setting members from outside the class. And `$baz = $foo->bar` *is* getting them from outside the class. When you have accessors for any member anyway, there is no proper information hiding anymore. It's the same as having all of them as public members. – Gordon Sep 21 '11 at 13:47
  • No, $foo->bar = 'baz' using a __set or __get method is calling that method, and is changing an already existing member inside the class. Anyway, even if I did like that kind of syntax, it's still no solution for me. As a stated before, the class is a little more complicated than just storing settings. For examples, settings can be saved to a file or a database, and if I just random add members its much harder to do that, since they're not the only member variables in the class. Since I also have more member variables I also can't use those names for settings. (no chars left, see next message) – Tiddo Sep 21 '11 at 13:48
  • And I also can't keep track of when settings are changed. Some settings might need to update other parts of the application, or need to be validated before they are changed. I can't do that without getters and setters. – Tiddo Sep 21 '11 at 13:49
  • @Tiddo well, obviously you have a design problem then. On the one hand you want the object to pose as a dumb data structure you can throw data random at. On the other hand you want to control how it gets saved and probably loaded and put *all that logic* into `__set` (which really is an error handler btw) for all kind of potential members inside the random data. Your object has too many responsibilities. Writing is one. Reading is another. And if some values require updating other values or validation, those Settings should likely get their own dedicated methods or objects. – Gordon Sep 21 '11 at 14:10
  • How it's saved and loaded isn't in the __set or __get methods, I have separate methods for that. Also, the settings class isn't a dumb datastructure to throw random data at, the array inside the settings class is. Primary the settings class needs to give access to the settings for other classes (settings array is static). The first time an instance is made, the settings need to be loaded from a file, I think it's best done inside this class. At some point the settings need to be saved back. Probably the easiest way to do it is to do that in the same class. (again, no more chars left...) – Tiddo Sep 21 '11 at 14:33
  • however, if you think it's not a good design could you please give me some advice how to do it better? This was just the way I thought it would be good, but of course I could be wrong (and looking at your reputation you probably know more about this than I do). – Tiddo Sep 21 '11 at 14:34
  • Your revised answer that analyzes the whole situation makes some very good points. It goes far far above and beyond what this question was asking. Some might see that as overkill itself, but many of the points you've brought out are well worth hashing and rehashing any time they come in purvey. There are a few _minor_ mistakes (arrays cannot be cloned, the file array var_export needs to include opening php – Zimzat Sep 22 '11 at 13:29
  • Thanks very much, this is a really helpfull answer. However, I do have a few remarks: 1. I'm probably not going to store the settings in a php file, so I'll have to create a class for loading the settings (or perhaps one class for loading and saving them, but than I'm giving one class multiple responsibilities again). 2. For the updating part: this was exactly what I meant the class should do (notify other classes), and I really like your way of implementation! It's much better than I would've done. (no characters left) – Tiddo Sep 23 '11 at 06:32
  • 3. You said "you cannot use $settings['foo']['bar'] with ArrayAccess" but if I look at you're code it looks like I can nest the HashMap class and therefore can use $settings['foo']['bar']. 4. I know singletons are bad and I should use dependency injection instead, however I think dependency injection will only make things more complicated in this case: Every class must be able to use the settings, but not every class will. There will be situations where I have a class C owned by class B owned by class A, but only class C uses the settings. (chars...) – Tiddo Sep 23 '11 at 06:41
  • With DI however I'll also have to hold a reference to the settings object in the A and B class, while they don't even use it. I'll have to pass the settings object to every single class in the entire application. In this case I think I can better use a static variable in the settings class, or a singleton, I think it's pretty useless to pass the settings object to every class in the application. However, thanks very much for your help! – Tiddo Sep 23 '11 at 06:44
  • @Tiddo No, you dont have to pass dependencies through the object graph. That is a common misconception. See http://stackoverflow.com/questions/6094744/dependecy-hell-how-does-one-pass-dependencies-to-deeply-nested-objects/6095002#6095002 – Gordon Sep 23 '11 at 07:33
  • That's the other way around: the door needs a lock and the house needs a door. Suppose a simple MVC framework: Suppose you have a core object which creates a controller and a settings object, and the controller creates a view. Suppose the view needs a settings, e.g. the backgroundcolor, from the settings object. But the settings object is created by the core object, so it needs to be passed to the controller first, so he can pass it to the view. Now the controller needs to have a pointer to the settings object even though he doesn't need it himself. (not the best example, but you get the idea) – Tiddo Sep 23 '11 at 10:26
  • @Tiddo Controller doesnt/shouldnt create the View. ControllerFactory creates Controller, creates Settings objects, creates View Object. Now you got the House example again: `new Controller(new View(new Settings))`. Have a look at these http://www.youtube.com/results?search_query=The+Clean+Code+Talks&aq=f – Gordon Sep 23 '11 at 10:34
  • But the outer object can't always know what the most inner object needs. Usually a view depends on a model, and a model is usually created by the controller (after all, that's more or less what a controller is for). The outer object doesn't know what model the view needs, that's not his job, so it can't create the view object, only the controller can. Or is there a better way to do that? – Tiddo Sep 23 '11 at 10:42
  • @Tiddo The controller shouldnt create the model either. Controllers only handle input from the User Interface and delegate to the appropriate Models. When you get a URL you usually already know what Controller and Model and View to instantiate so you create and wire all that into a RequestFactory. In the rare cases where you need to instantiate a different object based on some computation inside those collaborators, you have the RequestFactory inject another Factory just for that purpose and encapsulate the logic within that factory. Check the "Dont look for things" video I linked. – Gordon Sep 23 '11 at 10:55
  • Ahh ok. I think I begin to understand it. Thank you very much for your help! I'm learning more from you than from college! – Tiddo Sep 23 '11 at 11:04
2

The part $foo->bar is actually calling __get, this function should (in your case) return an array.

returning the right array in the __get would then be your solution.

OwenRay
  • 66
  • 3
  • I tried that, but than the array is just copied as a return value, so nothing is changed inside my class. – Tiddo Sep 21 '11 at 13:19
0

As has been stated, this is because it is the array stored in $foo->bar that is being modified rather than the class member. The only way to invoke __set behaviour on an 'array' would be to create a class implementing the ArrayAccess interface and the offsetSet method, however this would defeat the purpose of keeping the settings in the same object.

A reasonably neat and common work around is to use dot delimited paths:

class Settings {

  protected $__settings = array();

  // Saves a lot of code duplication in get/set methods.
  protected function get_or_set($key, $value = null) {
    $ref =& $this->__settings;
    $parts = explode('.', $key);

    // Find the last array section
    while(count($parts) > 1) {
      $part = array_shift($parts);
      if(!isset($ref[$part]))
        $ref[$part] = array();
      $ref =& $ref[$part];
    }

    // Perform the appropriate action.
    $part = array_shift($parts);
    if($value)
      $ref[$part] = $value;
    return $ref[$part];
  }

  public function get($key) { return $this->get_or_set($key); }

  public function set($key, $value) { return $this->get_or_set($key, $value); }

  public function dump() { print_r($this->__settings); }
}

$foo = new Settings();
$foo->set('somename', 'something');
$foo->set('bar.baz', 'somethingelse');
$foo->dump();
/*Array
  (
    [somename] => something
    [bar] => Array
      (
        [baz] => somethingelse
      )
  )*/

This also makes it clearer you are not manipulating instance variables, as well as allowing arbitrary keys without fear of conflicts with instance variables. Further processing for specific keys can be achieved by simply adding key comparisons to get/set e.g.

public function set(/* ... */) {
  /* ... */
  if(strpos($key, 'display.theme') == 0)
    /* update the theme */
  /* ... */
}
connec
  • 7,231
  • 3
  • 23
  • 26
  • This is way over-complicated for what the question is looking for, and that's not the only way. It's another way, but for this situation it's overkill. – Zimzat Sep 21 '11 at 13:56
  • I'm not certain it is. The functionality is reusable and it provides a clean interface for storing and retrieving settings with scope for additional processing, which is what the OP seems to be looking for. And I didn't claim it was the only way, I only claimed it was common (used in several frameworks including CakePHP). – connec Sep 21 '11 at 14:01
  • It's a nice solution, and I'd already thought of that (in fact, that was the first thing I came up with, before using the getter and setter methods). However, I like the getter and setter method better with the solution Zimzat gave me. – Tiddo Sep 21 '11 at 14:02
  • Fair enough. You should note though that there is no way, using that method, to trigger additional processing based on the array key as the particular key being accessed is not visible to __get/__set (e.g. `$foo->display['theme'] = new Theme;`; __get cannot tell you are intending to set 'theme'). – connec Sep 21 '11 at 14:10
  • I haven't thought of that... In that case I might have to use your method after all, since I need to do some checking on the settings. – Tiddo Sep 21 '11 at 14:37