php - Getting "Indirect modification of overloaded property has no effect" notice

Question

I want to use a Registry to store some objects. Here is a simple Registry class implementation.

<?php
  final class Registry
  {
    private $_registry;
    private static $_instance;

    private function __construct()
    {
      $this->_registry = array();
    }

    public function __get($key)
    {
      return
        (isset($this->_registry[$key]) == true) ?
        $this->_registry[$key] :
        null;
    }

    public function __set($key, $value)
    {
      $this->_registry[$key] = $value;
    }

    public function __isset($key)
    {
      return isset($this->_registry[$key]);
    }

    public static function getInstance()
    {
      if (self::$_instance == null) self::$_instance = new self();
      return self::$_instance;
    }
}

?>

When I try to access this class, I get "Indirect modification of overloaded property has no effect" notification.

Registry::getInstance()->foo   = array(1, 2, 3);   // Works
Registry::getInstance()->foo[] = 4;                // Does not work

What do I do wrong?

Answer 1

I know that this is now quite an old topic, but it is something that I encountered myself for the first time today, and I thought it might be helpful to others if I expanded upon what was said above with my own findings.

As far as I can tell, this is not a bug in PHP. In fact, I suspect the PHP interpreter has to make a special effort to detect and report this issue so specifically. It relates to the way in which you are accessing the "foo" variable.

Registry::getInstance()->foo

When PHP sees this part of your statements, the first thing that it does is check to see if the object instance has a publicly accessible variable called "foo". In this case, it doesn't, so the next step is to call one of the magic methods, either __set() (if you are attempting to replace the current value of "foo"), or __get() (if you are trying to access that value).

Registry::getInstance()->foo   = array(1, 2, 3);

In this statement, you are attempting to replace the value of "foo" with array(1, 2, 3), so PHP calls your __set() method with $key = "foo" and $value = array(1, 2, 3), and everything works fine.

Registry::getInstance()->foo[] = 4;

However, in this statement, you are retrieving the value of "foo" so that you can modify it (in this case by treating it as an array and appending a new element). The code implies that you want to modify the value of "foo" held by the instance, but in reality you are actually modifying a temporary copy of foo returned by __get(), and so PHP issues the warning (a similar situation arises if you pass Registry::getInstance()->foo to a function by reference instead of by value).

You have a few options for working around this issue.

Method 1

You could write the value of "foo" into a variable, modify that variable, and then write it back, i.e.

$var = Registry::getInstance()->foo;
$var[] = 4;
Registry::getInstance()->foo = $var;

Functional, but horribly verbose and so not recommended.

Method 2

Have your __get() function return by reference, as suggested by cillosis (there is no need to have your __set() function return by reference, since it is not supposed to return a value at all). In this case you need to be aware that PHP can only return references to variables that already exist, and may issue notices or behave strangely if this constraint is violated. If we look at cillosis' __get() function adapted for your class (if you do choose to go down this route then, for reasons that are explained below, stick with this implementation of __get() and religiously do an existence check before any read from your registry):

function &__get( $index )
{
    if( array_key_exists( $index, $this->_registry ) )
    {
        return $this->_registry[ $index ];
    }

    return;
}

This is fine provided your application never tries to get a value that doesn't yet exist in your registry, but the moment you do, you will hit the "return;" statement and get an "Only variable references should be returned by reference" warning, and you can't fix this by creating a fallback variable and returning that instead, since that will give you the "Indirect modification of overloaded property has no effect" warning again for the same reasons as before. If your program can't have any warnings (and warnings are a Bad Thing because they can pollute your error log and affect the portability of your code to other versions/configurations of PHP), then your __get() method would have to create entries that do not exist before returning them, i.e.

function &__get( $index )
{
    if (!array_key_exists( $index, $this->_registry ))
    {
        // Use whatever default value is appropriate here
        $this->_registry[ $index ] = null;
    }

    return $this->_registry[ $index ];
}

Incidentally, PHP itself seems to do something very similar to this with its arrays, that is:

$var1 = array();
$var2 =& $var1['foo'];
var_dump($var1);

The above code will (on at least some versions of PHP) output something like "array(1) { ["foo"]=> &NULL }", meaning the "$var2 =& $var1['foo'];" statement might affect both sides of the expression. However, I think that it is fundamentally bad to allow the contents of a variable to be changed by a read operation, because it can lead to some seriously nasty bugs (and hence I feel that the above array behaviour is a PHP bug).

For example, let us suppose that you're only ever going to store objects in your registry, and you modify your __set() function to raise an exception if $value is not an object. Any object stored in the registry must also conform to a special "RegistryEntry" interface, which declares that the "someMethod()" method must be defined. The documentation for your registry class thus states that a caller can attempt to access any value within the registry, and the result will either be retrieval of a valid "RegistryEntry" object, or null if that object does not exist. Let's also suppose that you further modify your registry to implement the Iterator interface so that people can loop through all registry entries using a foreach construct. Now imagine the following code:

function doSomethingToRegistryEntry($entryName)
{
    $entry = Registry::getInstance()->$entryName;
    if ($entry !== null)
    {
        // Do something
    }
}

...

foreach (Registry::getInstance() as $key => $entry)
{
    $entry->someMethod();
}

The rational here is that the doSomethingToRegistryEntry() function knows that it's not safe to read arbitrary entries from the registry, since they may or may not exist, so it does a check for the "null" case and behaves accordingly. All well and good. By contrast, the loop "knows" that any write operation to the registry would have failed unless the value written was an object that conforms to the "RegistryEntry" interface, so it doesn't bother to check to make sure that $entry is indeed such an object to save unnecessary overhead. Now let's suppose that there's a very rare circumstance under which this loop is reached sometime after an attempt is made to read any registry entry that does not yet exist. Bang!

In the scenario described above, the loop would generate a fatal error "Call to a member function someMethod() on a non-object" (and if warnings are Bad Things, fatal errors are Catastrophes). Finding out that this is actually being caused by a seemingly-innocuous read operation somewhere else in the program that was added by last month's update is not going to be straightforward.

Personally, I would avoid this method as well because whilst it can appear to behave well most of the time, it can really bite you hard if provoked. Happily, there is a much simpler solution available.

Method 3

Just don't define __get(), __set(), or __isset()! Then, PHP will create properties for you at runtime and make them publicly accessible, so that you can simply access them directly whenever you need to. No need to worry about references at all, and if you want your registry to be iterable, you can still do this by implementing the IteratorAggregate interface. Given the example you gave in your original question, I believe that this is by far your best option.

final class Registry implements IteratorAggregate
{
    private static $_instance;

    private function __construct() { }

    public static function getInstance()
    {
        if (self::$_instance == null) self::$_instance = new self();
        return self::$_instance;
    }

    public function getIterator()
    {
        // The ArrayIterator() class is provided by PHP
        return new ArrayIterator($this);
    }
}

The time to implement __get() and __isset() are when you want to give callers read-only access to certain private/protected properties, in which case you don't want to be returning anything by reference.

I hope that this helps. :)