configuration cannot be merge and must replace

[nykopol]

Merge options with array_merge_recursive lead to unusable configuration. For example, if we overwrite tinymce_jquery the resulting value is tinymce_jquery: [true, false] witch is a non-sense.

[MisatoTremor]

Unfortunately this leads to other problem cases.

Example:

$config = [
    'tinymce_jquery' => true, 
    'theme' => [
        'advanced' => [
            'theme' => 'modern',
            'plugins' => [
                'p1 p2',
                'p3 p4'
            ]
        ]
    ]
];
$options = [
    'theme' => [
        'advanced' => [
            'plugins' => [
                'p4 p5'
            ]
        ]
    ]
];
$config = array_replace_recursive($config, $options);

gives

Array
(
    [tinymce_jquery] => 1
    [theme] => Array
        (
            [advanced] => Array
                (
                    [theme] => modern
                    [plugins] => Array
                        (
                            [0] => p4 p5
                            [1] => p3 p4
                        )

                )

        )
)

No real easy way to solve all cases here.

[nykopol]

Yes.. and even if we do a good merge, it's impossible to determine if some values must be combined or overwritten.

This lead to the question : if we cannot handle correctly this, should we implement this ?

[MisatoTremor]

As the PR introducing this came from me, I'd say yes. :D

What do you think about:

public function tinymceInit($options = array(), $replace = false)
{
    $config = $this->getParameter('stfalcon_tinymce.config');
    if ($replace) {
        $config = array_replace_recursive($config, $options);
    } else {
        $config = array_merge_recursive($config, $options);
    }
    // ...
}

That way the user can decide which method fits.

[nykopol]

Yes, it look like a good solution. 👍

[Ingannatore]

I was also thinking about the configuration.

TinyMCE's one is pretty big and complex (see here), and maybe it will change from release to release; that's why I don't think it is the best solution to map it into our bundle's YAML configuration.

What do you guys think about converting all the children of the configuration YAML node into a JSON string and sending it directly to TinyMCE's init method? Something like

stfalcon_tinymce:
    # Other options ...
    configuration:
        language: "it_IT"
        selector: "textarea"

that becomes

tinymce.init({
    language: "it_IT"
    selector: "textarea"
});

Optionally we could add a simple validation for the children of our configuration YAML node like checking if the option exists, for example.

[RubenHarms]

👍

[MisatoTremor]

@Ingannatore This is exactly what we have now. The tinymceInit twig function gets the config from YAML, does some pre-processing and then JSON encodes it. This is processed in JavaScript depending if unsing TinyMCE with or without jQuery and then passed to init TinyMCE.

[zanardigit]

@nykopol if you implement the changes proposed by @MisatoTremor in #7 (comment), I'll be happy to merge since that will fix the immediate problem.

Further enhancements to the configuration handling will be postponed to future releases.

Thanks to everyone for the comments and contribution!

[Ingannatore]

@MisatoTremor yes, but it works only for the options defined inside the file src/DependencyInjection/Configuration.php; if I need to use, for example, the option content_style, I can't until I add its definition in the aforementioned file.

[nykopol]

@zanardigit Ok, i to that.
@Ingannatore @MisatoTremor I think that handling TinyMCE config as Yaml or Json without explicit known of this options in the bundle is a good enhancement since we delegated TinyMCE to a third party. But we should maybe do that in an other PR to avoid mixing of problematic ?

[MisatoTremor]

That's true, but you can at least do so by passing this config in twig.
Defining it per default is another issue of course and might be better discussed in its own issue.
One proposal though: TinyMCE 4s config should be relatively stable, so it should be sufficient to build that into yaml, defining most options as optional. But still I'd consider a config key additional_config for config options introduced by plugins as we can't possibly add them all. ;)

[nykopol]

@zanardigit it added proposal of @MisatoTremor with a small doc in the readme. I think this PR can be merge.