[#19052] - Modification to Joomla.loadOptions() so it actually merges option obj…
- Fixed in Code Base
- 14 Dec 2017
- Medium
- Build: 4.0-dev
- # 19052
- Diff
- Fabrik:4.0-dev-mergeoptions
- Pending hound Hound is busy reviewing changes... Details
User tests: Successful: Unsuccessful:
…ects.
Pull Request for Issue # .
Summary of Changes
Tweak to Joomla.loadOptions() so it actually merges option objects, rather than replacing them. The current code has a comment saying "Merge with existing", but as implemented it just replaces existing options with the new ones.
This change checks to see if existing and new options are objects (and not null), and if so, uses Joomla.extend() to merge the new one into the existing one.
The change is backward compatible with all current uses of Joomla.loadOptions in the CMS code, specifically by allowing a new option of 'null' to reset (as used by jtext).
Testing Instructions
In Chrome, edit an article on the backend. Open dev tools, and in the console look at Joomla.optionsStorage. You'll see a bootstrap.tabs options object, with a single entry (myTab).
Run this in the console (without this change):
Joomla.loadOptions({'bootstrap.tabs': {'hughTab': {'active': "foo"}}})
... and you'll see that the bootstrap.tab options object has been replaced with the new one, with just 'hughTab'.
Try the same thing with the change, and you'll get a bootstrap.tabs options object with both myTab and hughTab, as the options got merged.
This change isn't of much interest to "out of box" behavior, but my extension does a lot of adding of DOM content (including objects with options) on the fly, where I need to make successive calls to addOptions, and have the options add to, not replace, existing options.
Expected result
Actual result
Documentation Changes Required
Don't think so, as I think it just makes loadOptions behave how the internal comment says it does.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | JavaScript |
| Labels |
Added:
?
|
||
PS, sorry about the tabs vs spaces thing, I've now set PHP Storm to use smart tabs, so any future PR's from me should get it right. :)
Is there a J! Javascript code style XML for PHPStorm? The one I have only defines tabs for JS (which I'd managed to override somehow).
Just FYI, I have another PR waiting to go, which adds @Fedik 's joomla:updated event handling to bootstrap-init.js, but that one (indirectly) relies on this one, so the code in my extension (Fabrik) that needs bootstrap-init to parse new DOM content can make successive loadOptions() calls.
I'm not religious about how we do this - maybe rather than changing loadOptions(), have a mergeOptions() and maybe a clearOptions() (that takes a single name to clear). But I do think we need the ability to actually merge individual option objects, rather than just resetting them.
Use tabs and not spaces for indentation.
@cheesegrits - Don't worry about it in this PR. We'll do all that at a later date
@cheesegrits thanks! In general looks good.
The current code has a comment saying "Merge with existing", but as implemented it just replaces existing options with the new ones.
yeah, that what was planned. "Merge with existing" is actually merging only a root keys, and so replace old
but what you did is a better idea
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-12-14 19:47:10 |
| Closed_By | ⇒ | wilsonge |
Thanks Hugh :)
Another small change is to have Joomla.extend() behave like jQuery.extend(), and treat a null destination as an empty object, rather than erroring out. As well as just being The Right Thing To Do, it allows us to clear an option with Joomla.loadOptions({'foo.bar':null}), and later merge new options with Joomla.loadOptions({'foo.bar': {...}}) without it erroring out.