Closed glensc closed 5 years ago
After careful consideration I concluded this feature does not align with interface provided by zend-config
: Specifically, it bypasses read-only flag control and a guarantee that Zend\Config\Config
won't have a plain array as a value.
There are a few code quality related red flags for me as well: It introduces a side channel for modifications we have no control over. Side channels are a big contributor to subtle bugs in my experience and generally a maintenance nightmare. It promotes usage that goes against principle of least astonishment and is not generally safe as it can produce a code that looks proper, thus making it unreasonably hard to spot a problem while reading the code.
From your referencing PR, from documentation:
- if (count($config['ftp']) && !$config['ftp']['password'] instanceof EncryptedValue) {
- $config['ftp']['password'] = new EncryptedValue(
- CryptoManager::encrypt($config['ftp']['password'])
- );
- }
+ $event->encrypt($config['ftp']['password']);
Looking at the code without additional context I would expect it to perform some encryption operation using the password, but not that password itself would be changed. This is an actual example of violation of principle of least astonishment. Intention to simplify that code is understandable, however, consider following as an alternative to using pass-by-reference:
+ if (isset($config['ftp']['password']) {
+ $config['ftp']['password'] = $event->encrypt($config['ftp']['password']);
+ }
This code makes it very clear that after its execution password
value would be encrypted. It is also a pretty natural way for PHP developer to write code, so it would not even need special documentation example.
public function encrypt($value) : EncryptedValue
{
if ($value instanceof EncryptedValue) {
return $value;
}
return (new EncryptedValue())->setValue($value);
}
With pass-by-reference dropped from the function here, code is no longer concerned with changing value for the caller, that responsibility sits where change is happening. Code gets simpler and more predictable. It is usable in a wider range of applications. It can be expressed as a contract provided by interface and enforced by it. Indirect benefit of outcome guarantee shifted to interface is that this piece of code can be mocked safely by the consumer for more reliable unit testing.
@Xerkus ok, perhaps the leading example is bad. but don't get stuck by my given example. this problem also appears when trying to create subkeys that do not have a parent. it works with native arrays but not with zend\config overload. the linked StackOverflow post also contains scenarios that people are having.
the example is something like:
// cat 59.php
<?php
$config['some']['key'] = 'a value';
print_r($config);
unset($config);
require 'vendor/autoload.php';
$config = new Zend\Config\Config([]);
$config['some']['key'] = 'a value';
print_r($config->toArray());
with native arrays, the result is $config['some']['key'] == 'a value';
but with zend\config overload the Indirect modification of overloaded element of Zend\Config\Config has no effect
warning is given and result is empty array`.
$ php 59.php
Array
(
[some] => Array
(
[key] => a value
)
)
PHP Notice: Indirect modification of overloaded element of Zend\Config\Config has no effect
Array
(
)
$config = new Zend\Config\Config([]); $config['some']['key'] = 'a value'; print_r($config->toArray());
That would not work with the change from this PR either. There is nothing to return by reference for
'some'
, so there is nowhere to set new value.
What pass-by-reference is solving is a non-issue already as zend-config decorates arrays recursively:
$config = new Zend\Config\Config(['some' => []], true);
$config['some']['key'] = 'a value';
print_r($config->toArray());
// Array
// (
// [some] => Array
// (
// [key] => a value
// )
// )
ok, need to re-check what is going on. but at least the warning is gone, but somewhy doesn't solve the actual problem which i thought the patch did fix.
➔ git checkout config-reference-access
➔ php 59.php
Array
(
[some] => Array
(
[key] => a value
)
)
Array
(
)
ps: initializing parent key kind of "solution" works without the patch as well. yet it doesn't make it symmetrical with native php array access.
This adds resolution to typical notice seen:
Example:
This requires PHP 5.3.4 :