helios-ag / FMBbCodeBundle

:capital_abcd: BBCode bundle for Symfony projects
Other
57 stars 35 forks source link

[Insight] The Symfony Dependency Injection Container should not be passed as an argument - in Decoda/Hook/EmoticonHook.php, line 56 #122

Closed helios-ag closed 7 years ago

helios-ag commented 9 years ago

in Decoda/Hook/EmoticonHook.php, line 56

A Symfony dependency injection container has been found as an argument.

     *
     * @param LoaderInterface $loader   A LoaderInterface instance
     * @param mixed           $resource The main resource to load
     * @param array           $options  An array of options
     */
    public function __construct(LoaderInterface $loader, ContainerInterface $container, array $options = array())
    {
        $this->loader    = $loader;
        $this->container = $container;
        $this->setOptions($options);

Posted from SensioLabsInsight

alquerci commented 9 years ago

There is a good reason to inject the container because we need them to replace placeholders [ref] like on the Symfony\Bundle\FrameworkBundle\Routing\Router [ref].

helios-ag commented 9 years ago

Another approach is to inject parameters directly, so we can process them inside hook. Yaml file can be read by Yaml\Parser. What is your opinion, @alquerci ?

alquerci commented 9 years ago

inject parameters directly

Indeed here we just need of all container parameters but it is not possible to inject all of them.

Yaml file can be read by Yaml\Parser.

I do not understand you idea with this sentence. A parser can only parse and do not anything else.

This is the exception to the rule of not injecting the container.

The behind feature is a way to use all container parameters to avoid duplicate configuration like shown on the README:

# path/to/emoticons.yml
imports:
  - { resource: path/to/another/emoticons.yml }

emoticons:
  my_emoticon:
    url:   # Default: %fm_bbcode.emoticon.path%/my_emoticon.png
    html:  # Default: <img src="%fm_bbcode.emoticon.path%/my_emoticon.png" alt="" >
    xHtml: # Default: <img src="%fm_bbcode.emoticon.path%/my_emoticon.png" alt="" />
    smilies:
      - ":my_emoticon:"

In order to keep back compatibility we cannot remove this feature.

But it's possible that is only useful to resolve the fm_bbcode.emoticon parameter namespace. In that case we can inject only all needed parameters to resolve them for the next major version (it is a regression).

helios-ag commented 9 years ago

I do not understand you idea with this sentence. A parser can only parse and do not anything else.

My idea was to address emoticon file via fm_bbcode.emoticon parameter, and then process it, so we were able to avoid inject whole container.

This is the exception to the rule of not injecting the container.

Ok i got it.