Skip to content

Comments

Merge Script Options recursive#19972

Merged
mbabker merged 1 commit intojoomla:stagingfrom
bembelimen:patch-39
Aug 21, 2018
Merged

Merge Script Options recursive#19972
mbabker merged 1 commit intojoomla:stagingfrom
bembelimen:patch-39

Conversation

@bembelimen
Copy link
Contributor

Summary of Changes

With the JDocument::addScriptOptions method it's possible to change script parameters like the TinyMCE plugin parameters from outside (like with a plugin). This functionality was implemented with the following PR: #3072 and in the TinyMCE here: #11157 (with examples)

The problem at the moment is, that the "new parameters" will only be merged on the first level, so for example for the TinyMCE you cannot change parameters of deeper level in an easy way.

Testing Instructions

Add the following code into your template index file:

$options = [
    'foo' => [
        'bar' => [
			'bla' => 1,
		],
		'baz' => [
			'blub' => 2
		]
    ]
];

JFactory::getDocument()->addScriptOptions('Foobar', $options);

// Update the foo => bar => bla value
$options = [
	'foo' => [
        	'bar' => [
			'bla' => 2,
		]
	]
];

JFactory::getDocument()->addScriptOptions('Foobar', $options);

Expected result

    "Foobar": {
        "foo": {
            "bar": {
                "bla": 2
            },
            "baz": {
                "blub": 2
            }
        }
    }

Actual result

    "Foobar": {
        "foo": {
            "bar": {
                "bla": 2
            }
        }
    }

More description

This example is very small, but e.g. the tinyMCE has somewhere about 50 parameters with a level of 3 or more. If you want to change one parameter in the depth, you have to rebuild the whole tree (or load the current tree, setup the variables and then set again), which makes the "merge" a bit useless.

Perhaps @Fedik and @DGT41 could look over this functionality, because they worked with it.

@dgrammatiko
Copy link
Contributor

@bembelimen I like it and I don't see any problems changing the behaviour. Edge case if someone used this code to actually replace the values in the storage but then again I don't think that many people are actually using or even understand the Joomla.storageOptions. Anyhows I'm happy with this change but I guess @Fedik has the final say here!

@bembelimen
Copy link
Contributor Author

Thank you for your feedback @DGT41 if someone wants to replace, he could set the $merge parameter to "false"

@chmst
Copy link
Contributor

chmst commented Mar 24, 2018

I have tested this item ✅ successfully on d8b3c51

I've tested this item successfully on a 3.6.8 installation, following the testing instructions.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19972.

@Fedik
Copy link
Member

Fedik commented Mar 24, 2018

looks fine

the "new parameters" will only be merged on the first level

that was intent 😉
to keep stuff simple, and to force developer to use: getOptions() => modify an options array => setOptions(),

if it works, then can be as @bembelimen suggested also 😉

@HRIT-Florian
Copy link
Contributor

I have tested this item ✅ successfully on d8b3c51


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19972.

@ghost
Copy link

ghost commented Aug 20, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added RTC This Pull Request is Ready To Commit PR-staging and removed RTC This Pull Request is Ready To Commit labels Aug 20, 2018
@bembelimen bembelimen deleted the patch-39 branch December 30, 2018 23:54
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

Successfully merging this pull request may close these issues.

7 participants