Open alexh-swdev opened 2 months ago
I'd prefer array_key_exists()
In my personal older fix for Select, I indeed used array_key_exists()
because I also prefer it. However, in Field.php there already is a isset()
check for the array key so I had the impression OM does (or at least Magento did) prefer isset
over array_key_exists
.
I was curious about the actual difference so I looked up https://stackoverflow.com/a/3210982 . So the use of isset
made sense and I used this.
But in the end, I don't care. If you insist I'll change it?
I try to avoid isset()
whenever it is possible and use is_null()
or array_key_exists()
or similar. For me it makes it better readable. Since php7.4 it makes no difference in performance.
....hope you have write access :p
The description of this PR:
Sometimes, the array key "value" is not available, showing warnings in the logfile
The warning happens because there are mistakes in supplying the options in custom grids and forms. When rendering the HTML select element, the rendering functions expect the var $option
to have 2 keys: value
and label
. If these keys are absent, exception should be thrown. The warnings in the log file are very helpful and should not be silenced. Instead, the code upstream that causes the error should be corrected. (We shouldn't accept bad code from third party or core.)
Therefore, I object to this PR.
As I mentioned in https://github.com/OpenMage/magento-lts/pull/4125#issuecomment-2266627620 I think OM should handle this in a proper way. "Proper way" may be arguable:
Imho, the current way is neither fish nor fowl....
I am not sure:
Showing the option as good as possible (like this PR is done right now)
Issue as I described earlier: The warnings in the log file are very helpful and should not be silenced..
Filtering it out and don't show the option at all (alternative, like written in the code comment)
This is the same as silencing the warning.
Throw an exception like you proposed.
Potential BC, affected grids will no longer work.
Consider this: PHP 7 and earlier, it throws E_NOTICE
. PHP 8, it throws E_WARNING
. PHP 9, it'll throw E_ERROR
. I would let PHP handles it. Because, there is bug in caller code upstream that should be corrected.
We are concern about compatibility with PHP8 and there were many PRs on this. But this case is not about compatibility. Is it?
How about to validate the config and log what's wrong?
How about to validate the config and log what's wrong?
Nice idea. But if there is no developer, the log would just pile up. However if ($isDevelopMode) $adminSession->addWarning($warn)
?
Now, the log piles up, too, with the warnings ;) (ok, depending on cfg)
So I guess, this is not really solvable without BC, be it filter out wrong options and/or throwing exceptions, in 20.x, maybe it can be done in 21.x?
Description (*)
Sometimes, the array key "value" is not available, showing warnings in the logfile. This PR adds some isset-checks to avoid them.
Related Pull Requests
Fixed Issues (if relevant)
See for example
Warning: Undefined array key "value" in /var/www/XXX/app/code/core/Mage/Adminhtml/Block/System/Config/Form/Field.php on line 86
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)