Fixes for Sonar and PhpStan level 10 #2
Conversation
| case static::CONFIG_KEY_WEBSITES: | ||
| case static::CONFIG_KEY_STORES: | ||
| [$unused1, $unused2, $storeCode, $section, $group, $field] = $configKeyParts; | ||
| case self::CONFIG_KEY_WEBSITES: |
There was a problem hiding this comment.
I think I made these intentionally static so that the helper may be overridden.
There was a problem hiding this comment.
Why to override them? imho the config-keys should not change. (isnt it used in some regex?)
There was a problem hiding this comment.
Yes, hence the regex also use static::. I can not predict the reason someone wants to change the names. Maybe it breaks with their enviroment tooling or something.
| unset($configKeyParts[0]); | ||
|
|
||
| $scope = array_shift($configKeyParts); | ||
| if (!is_string($scope)) { |
There was a problem hiding this comment.
How would that ever be the case, coming from _ENV ?
There was a problem hiding this comment.
Guess it was for phpstan, but @var string $scope will also work. Update later.
Ty for review.
| return preg_match($regexp, $configKey); | ||
| $validatedConfigKey = preg_match($regexp, $configKey); | ||
| if ($validatedConfigKey === false) { | ||
| throw new Mage_Core_Exception( |
There was a problem hiding this comment.
Why does it throw now? It not caught and should not throw imho.
There was a problem hiding this comment.
You are right, be we could log it?
There was a problem hiding this comment.
Hmm.. yea I could see that being useful.
That ENV key starts with the correct prefix, but somewhere down the line a typo happened. That could be useful to the developer.
There was a problem hiding this comment.
Remove it to avoid more complexity. Maybe change later?
| protected const CONFIG_KEY_DEFAULT = 'DEFAULT'; | ||
| protected const CONFIG_KEY_WEBSITES = 'WEBSITES'; | ||
| protected const CONFIG_KEY_STORES = 'STORES'; | ||
| public const ENV_STARTS_WITH = 'OPENMAGE_CONFIG'; |
There was a problem hiding this comment.
Why are these public now? Does it help when it gets a class rewrite?
There was a problem hiding this comment.
imho the constants should be availble from outside.
There was a problem hiding this comment.
For what purpose? To set from the outside a class rewrite should be used in the magento space.
There was a problem hiding this comment.
No special purpose atm. All constants in helpers are public, why not here?
There was a problem hiding this comment.
Hm I am not seeing it anymore. Its okay to change visibility, if you think its correct. Or maybe we should move away from constants all together, since it makes it too complicated. No other tooling should need to use these constants, so they could be public methods just the same.
Or?
|
This was good to go now, right? |
...