TYPO3-Caretaker / caretaker_instance

TYPO3 extension caretaker_instance
GNU General Public License v2.0
14 stars 23 forks source link

alternatives for "clone $GLOBALS" ? #78

Open mario-seidel opened 4 years ago

mario-seidel commented 4 years ago

Can we think about this again?

https://github.com/TYPO3-Caretaker/caretaker_instance/commit/e00f12adcda66ba417899e7f1e046715206e9601#diff-85035a0cade545b687554eb75b5a191cR112

TehTux commented 4 years ago

Of course, can you give me input? I'm not familiar with the issue.

What is the issue? How can I reproduce it?

rengaw83 commented 4 years ago

Hi, the problem is, that $GLOBALS is an array and can't be cloned.

This code results in an fatal error:

$copy = clone $GLOBALS;

Output in ALL PHP versions:

Fatal error: Uncaught Error: __clone method called on non-object

To clone an array you just need to assign it to a new variable.

But $GLOBALS has a peculiarity, see https://www.php.net/manual/de/reserved.variables.globals.php#112395
The keys of $GLOBALS are always an reference!

To create a clone of the $GLOBALS array you can perform an array copy.

Here are a demonstration of this issue (or run it on https://3v4l.org/VkDgZ):

// Testing Globals
$GLOBALS['A'] = 'B';

$arrayCopyGlobalsVar = $GLOBALS;
$arrayCopyGlobalsVar['A'] = 'C';

var_dump([
    '$GLOBALS' => $GLOBALS['A'], 
    '$arrayCopyGlobalsVar' => $arrayCopyGlobalsVar['A'], 
]);

$GLOBALS['A'] = 'B';

$nonReferencedGlobalsVar = new ArrayObject($GLOBALS);
$nonReferencedGlobalsVar = $nonReferencedGlobalsVar->getArrayCopy();
$nonReferencedGlobalsVar['A'] = 'D';

var_dump([
    '$GLOBALS' => $GLOBALS['A'], 
    '$nonReferencedGlobalsVar' => $nonReferencedGlobalsVar['A']
]);

Output for PHP 5.4.0 - 7.4.4:

array(2) {
  ["$GLOBALS"]=>
  string(1) "C"
  ["$arrayCopyGlobalsVar"]=>
  string(1) "C"
}
array(2) {
  ["$GLOBALS"]=>
  string(1) "B"
  ["$nonReferencedGlobalsVar"]=>
  string(1) "D"
}

i can't understand how https://github.com/TYPO3-Caretaker/caretaker_instance/commit/e00f12adcda66ba417899e7f1e046715206e9601 could be done and why this should fix #58.

I think you can reproduce this by checking an extension value like mentioned in #58.

But i think you can replace this clone by doing this:

-                    $value = clone $GLOBALS;
+                    $value = (new ArrayObject($GLOBALS))->getArrayCopy();
mario-seidel commented 4 years ago

The main problem is, that a method called "getValueForKeyPath" is modifying the global state:

if ($keyPath[0] == 'TYPO3_CONF_VARS' && $keyPath[1] == 'EXT' && $keyPath[2] == 'extConf' && $keyPath[3]) {
    $value = clone $GLOBALS;
    $serializedValue = $value[$keyPath[0]][$keyPath[1]][$keyPath[2]][$keyPath[3]];
    $value[$keyPath[0]][$keyPath[1]][$keyPath[2]][$keyPath[3]] = unserialize($serializedValue);
}

With the if check the structure of the return array is clear. So we don't have to overwrite the $GLOBALS here. Instead we can return an array with the serialized data like this:

//untested code
if ($keyPath[0] == 'TYPO3_CONF_VARS' && $keyPath[1] == 'EXT' && $keyPath[2] == 'extConf' && $keyPath[3]) {
    $serializedValue = $GLOBALS['TYPO3_CONF_VARS']['EXT']['extConf'][$keyPath[3]];
    $value = [
        'TYPO3_CONF_VARS' => [
            'EXT' => [
                'extConf' => [
                    $keyPath[3] => unserialize($serializedValue)
                ]
            ]
        ]
    ];
}

In fact, this is much more readable and do what the method name pretends.

TehTux commented 4 years ago

Hey guys,

thanks for your detailed feedback. Do you need a backport for 2.x?

mario-seidel commented 4 years ago

There are still some needs for it so it would be really nice to have a backport.