Closed nykopol closed 9 years ago
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.
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 ?
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.
Yes, it look like a good solution. :+1:
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.
:+1:
@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.
@nykopol if you implement the changes proposed by @MisatoTremor in https://github.com/gibilogic/tinymce-bundle/pull/7#issuecomment-138520205, 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!
@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.
@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 ?
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. ;)
@zanardigit it added proposal of @MisatoTremor with a small doc in the readme. I think this PR can be merge.
Merge options with array_merge_recursive lead to unusable configuration. For example, if we overwrite
tinymce_jquery
the resulting value istinymce_jquery: [true, false]
witch is a non-sense.