fix some 'undefined array key' warnings

[alexh-swdev]

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 (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[sreichel]

I'd prefer array_key_exists()

[alexh-swdev]

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?

[sreichel]

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.

[alexh-swdev]

....hope you have write access :p

[kiatng]

What's the reason for setting label (instead of empty string) to option value?

[alexh-swdev]

I did it, so each option has a different / its own value, what the 'value' key in the map was supposed to provide.
If you think that's not required, we can leave the value attribute of the tag empty.

[sreichel]

I'd prefer an empty string.

Maybe you can share the extension that causes the problem, to reproduce it?

[alexh-swdev]

Ok, I'll change it to empty string then.
It's an Amazon Pay add-on from creative style. I hope I can provide it in the next days

[sreichel]

Maybe its better to fix the extension itself. There some forks of it ...

https://github.com/kirchbergerknorr/magento1_creativestyle_amazon-payments
https://github.com/mothership-gmbh/magento1_creativestyle_amazon_payments

Or create a repo, or better update https://github.com/OpenMageModuleFostering/creativestyle_amazonpayments

[kiatng]

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.

[alexh-swdev]

As I mentioned in #4125 (comment) I think OM should handle this in a proper way.
"Proper way" may be arguable:

• Showing the option as good as possible (like this PR is done right now)
• Filtering it out and don't show the option at all (alternative, like written in the code comment)
• Throw an exception like you proposed.

Imho, the current way is neither fish nor fowl....

[kiatng]

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?

[sreichel]

How about to validate the config and log what's wrong?

[kiatng]

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)?

[alexh-swdev]

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?