[php8.1] deprecated passing null to str_replace() , ref #1812 #2806
Conversation
| @@ -133,7 +133,7 @@ public function loadToXml(Mage_Core_Model_Config $xmlConfig, $condition = null) | |||
| $deleteStores = []; | |||
| // set stores config values from database | |||
| foreach ($rowset as $r) { | |||
| if ($r['scope'] !== 'stores') { | |||
| if (is_null($r['value']) || $r['scope'] !== 'stores') { | |||
There was a problem hiding this comment.
Didn't test, but in PHP7, if (is_null($r['value']) then $value = empty string, and it would continue to line 40. Shouldn't PHP8 do the same?
There was a problem hiding this comment.
Can you please test it?
Your right, this changes some behavoir ... we dont set empty nodes ...
Test
- set some config to null in DB (i've used Title Prefix form Admin -> Design config)
- load/save config
- set debug breakpoint at
Varien_Simplexml_Config::setNode()
There was a problem hiding this comment.
I'll look into it after my holiday.
There was a problem hiding this comment.
and it would continue to line
Right, but there we just create an empty XML-node for a null DB value.
With this change the node is not created. What could happen? Access a not-existing node (getConfigNode() or getNode())? Did not found any errors yet.
I could be an improvement to not create emtpy xml-nodes, but needs some testing ...
I'll open a new PR with casting to string to move forward. This can be investigated later. :)
There was a problem hiding this comment.
I'm thinking of an edge case which I'm not sure if it exists or not: What if there's a config that has a default value in xml config file and you clear the value from Admin panel, so it tries to save null in the database but that won't happen. So it will keep using the default value in xml file. Please let's investigate this before proceeding.
There was a problem hiding this comment.
@elidrissidev tested this and everything looks fine. (Method description says Load configuration values into xml config object and we just avoid loading null to config object.)
Edit:
Default valuew from config.xml are only used when no config path exists in core_config_data. It is not used when a null entry exists in DB.
If i did not wrong this PR works as it should,
There was a problem hiding this comment.
Default valuew from config.xml are only used when no config path exists in
core_config_data. It is not used when anullentry exists in DB.
@sreichel where did you see that it's not used if value in db is null?
There was a problem hiding this comment.
I have tested this by setting contacts/email/recipient_email to NULL in db (it has default value of "hello@ example.com" in xml), and then running:
Mage::app()->getConfig()->getNode('default/contacts/email/recipient_email')I could see that before adding the is_null($r['value']) check, it was returning an empty string value, whereas after that it's returning the value from xml. So might not be totally safe.
There was a problem hiding this comment.
Thanks. Tested getConfigNode() at the beginning - w/o default config value.
|
This is the first error i see, when testing php8.1 and dont supress errors. Review needed. See comments above. |
sreichel
added
needs investigation
review needed
|
Better use #2901 |
Description (*)
Related Pull Requests
Fixed Issues (if relevant)
Deprecated functionality: str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated in app/code/core/Mage/Core/Model/Resource/Config.php on line 104Questions or comments
Fixed bit different. Set no default value when value from DB is NULL.
Contribution checklist (*)