corcel-acf icon indicating copy to clipboard operation
corcel-acf copied to clipboard

Backwards compatible optimization for large options.

Open Stevemoretz opened this issue 3 years ago • 1 comments

Before this PR, you couldn't grab just one specific field, sometimes you have large option pages and you only want one field for instance :

OptionFieldGroup::byTitle('Manual Rates')->first()->loadOptions("options")->getOption("exchange")->toArray(),

Would get all the options of Manual Rates from the DB, but :

OptionFieldGroup::byTitle('Manual Rates')->first()->loadOptions("options", "exchange")->getOption("exchange")->toArray(),

Only looks for options_exchange_* that means all the other fields, won't be selected and retrieved, this arises one question in mind, what happens in caching? well I added a function to invalidate caches :

OptionFieldGroup::byTitle('Manual Rates')->first()->invalidateCache();

There is no breaking changes just a few things added that's all!

Stevemoretz avatar Aug 05 '22 17:08 Stevemoretz

Hi, first of all thanks for your contribution!

Could you explain why the change is necessary? Is it for performance/memory reasons? Loading all the options only takes one DB query (as you have seen in the code), so that should not be a problem? And if your code does not access the unneeded fields, no additional processing takes place (just setLocalKey() and setData(), which are both cheap to call). Do you see any performance gains in your PR?

I am a bit hesistant to merge PR as it is, because it could lead to nasty cache problems. Say, you have a large application, in which different parts of the code access the same option page. A single part of the code can not know, if the option page has been loaded completly, or just certain fields (with the new $filter). So any access to that option page would need to call invalidateCache() beforehand, rendering the cache useless.

If you really need this, I would suggest something like the following:

  • create a new method getOptionUncached() (or a parameter $skipCache in getOption), which bypasses the cache completly and loads a certain field from the database
  • most of the code of loadOptions() would be put into a new method loadOptionsFromDb(), which just returns the list of options and leaves $this->loaded and $this->options alone (your filter logic could be added there)
  • loadOptions() just calls loadOptionsFromDb() (without a filter) and sets $this->options and $this->loaded as it does now
  • the new method getOptionsUncached() always calls loadOptionsFromDb() (with a filter) and ignores the cache completly
  • invalidateCache() can be removed

Happy to hear your feedback!

tbruckmaier avatar Sep 15 '22 11:09 tbruckmaier