Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loadString() behaviour #50

Open
vess opened this issue Nov 26, 2018 · 3 comments
Open

loadString() behaviour #50

vess opened this issue Nov 26, 2018 · 3 comments
Labels

Comments

@vess
Copy link

vess commented Nov 26, 2018

Steps to reproduce the issue

$config = new Registry;
$config->set('options', $someObj);
$config->loadFile($file);

Expected result

$someObj + data from file

Actual result

Only data from file since $this->initialized still is false after calling set method.
loadString has a check for $this->initialized and overwrites existing data that was set.

Is it intended that we have to call one of the load methods first to get $this->initialized set to true to get them merged or can we get rid of that if statement?

@mbabker
Copy link
Contributor

mbabker commented Nov 26, 2018

Is it intended that we have to call one of the load methods first to get $this->initialized set to true to get them merged or can we get rid of that if statement?

The check needs to stay in place because it creates a performance optimization to be able to overwrite the data store versus merging data. So the checks need to be improved.

@vess
Copy link
Author

vess commented Nov 26, 2018

But right now the check is only in loadString, loadObject and loadArray call bind directly...
and if you create the data store from object or array and then do a loadFile/loadString it is merged...

@mbabker
Copy link
Contributor

mbabker commented Nov 26, 2018

That's part of the performance optimization. When a string is being loaded then internally converted to a stdClass object, if the data store hasn't been initialized at all yet then we can direct assign that converted string as the data store. Removing that if conditional removes the performance enhancement as at that point it will hit the bindData() method which manually merges each key.

So, again, the checks need to be improved. The short circuiting shouldn't be removed, and if it makes sense the short circuiting should be added to the other load methods (with the bug about the initialized flag not being set correctly based on your scenario being fixed).

@nibra nibra added the bug label Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants