Ensure DeploymentConfig Reader always returns an array #13584
Conversation
|
|
There was a problem hiding this comment.
What do you think about throwing an exception instead of silently ignoring an invalid configuration files?
I mean, right after the configuration file inclusion.
There was a problem hiding this comment.
But that would still block the deployment process as described in #13090
There was a problem hiding this comment.
The current behavior when $fileKey is empty is actually also silently ignoring invalid files: https://github.com/barryvdh/magento2/blob/e4d857fe63717d24da7b3fbeaa44504fcd866632/lib/internal/Magento/Framework/App/DeploymentConfig/Reader.php#L115
So this would bring it inline with that behaviour.
Or do you want me to add check after both includes to throw an exception?
There was a problem hiding this comment.
It's fine to block deployment process in case of invalid data provided. Otherwise it would be pretty nontrivial to debug what went wrong.
Yeah, looks like is_array was randomly introduced in barryvdh@02596b7#diff-db4cf46912b0aad3f54619ff946b9ac8R115, please throw proper exception in both cases when file data is invalid.
There was a problem hiding this comment.
Please add corresponding @throws line into method PHPDoc.
There was a problem hiding this comment.
Please replace to an equivalent if ($fileData). As always, please squash changes into a single commit.
There was a problem hiding this comment.
Done, done and done.
Throw exception on invalid config
4 participants
Description
When a empty configuration file is ready, this could result in loading invalid configuration, eg. just
1instead of an array. This will lead to problems with other code, expecting a valid array.Fixed Issues
/pull/13090
Manual testing scenarios
Create an empty file as app/etc/env.php and run the CLI installer