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

bug: multiple setExtraColumns calls cause issues with values not being cleared #175

Closed
rshkabko opened this issue Feb 17, 2023 · 4 comments

Comments

@rshkabko
Copy link

For example, I have multiple user portals. I use Setting::setExtraColumns(['portal_id' => $portal_id]);
For example, I want to get (or update) some settings on all portals (or even 2 at once).
\Setting::setExtraColumns(['portal_id' => 15]);
$setting = \Setting::get('EXCHANGE'); // OK, will get portal 15 setting

\Setting::setExtraColumns(['portal_id' => 20]);
$setting = \Setting::get('EXCHANGE'); // BUG, will get portal 15 setting TOO

All because you use here:

public function load($force = false) { if (!$this->loaded || $force) { $this->data = $this->readData(); $this->persistedData = $this->data; $this->data = array_merge($this->updatedData, $this->data); $this->loaded = true; } }

This

if (!$this->loaded || $force) {

Please, optimize or remove it.
I hope you understand)

rshkabko added a commit to rshkabko/settings that referenced this issue Feb 17, 2023
@rshkabko
Copy link
Author

I spent about 7 hours to understand why my settings are not working))

@bweston92 bweston92 changed the title Bug when use foreach bug: multiple setExtraColumns calls cause issues with values not being cleared Feb 18, 2023
@bweston92
Copy link
Collaborator

Hi @rshkabko please correct me if I am wrong but from what I can gather you're using Setting::setExtraColumns(['portal_id' => $portal_id]); multiple times in one execution and due to the load method using a local cache it overrides the previous values.

This wasn't a use case we had thought about but the following should work:

Option 1

Force reload after changing the columns.

foreach (...) {
    Setting::setExtraColumns(['portal_id' => $portal_id]);
    Setting::load(true);
    ...
}

Option 2

Write your own SettingStore that will cater for this, you can do this by extending the base class of the SettingStore you currently use and overwriting the function load. The custom SettingStore can then be configured as your setting store.

Option 3

Write a PR that resets a SettingStore's state when setExtraColumns is called.

@bweston92
Copy link
Collaborator

Option 3 has been mentioned before #32 (comment)

Feel free to create a PR or use any other suggested option.

rshkabko added a commit to rshkabko/settings that referenced this issue Feb 23, 2023
rshkabko added a commit to rshkabko/settings that referenced this issue Feb 23, 2023
@rshkabko
Copy link
Author

Hello!
Fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants