Added reflection to ThemeCompilerPass to address changes made in symf…

[fotomerchant]

Added reflection to ThemeCompilerPass to address changes made in symfony/symfony@2d42689 which should address #157

[fotomerchant]

In terms of the tests, I didn't have time to figure out how to create the appropriate Mocks for the ThemeCompilerPass since it's now using Reflection. If anyone has any ideas I'll take them :)

[Sander-Toonen]

Symfony 3.2.1 just got released, resulting in #157.

Tests are broken but I can confirm that the PR successfully solves the issue when running Symfony 3.2.1, also it doesn't break BC (tested in Symfony 3.2.0)

[fotomerchant]

Yeah, the tests are broken because my method of fixing it was to enhance the ThemeCompilerPass with reflection. However, my experience with creating Mocks in unit tests is limited and I haven't had the time to figure out how to create a Mock complex enough to pass the unit tests.

If there is someone out there with more experience with Mocks then they might be able to get it working and then I can add a few more tests to handle both pre and post Symfony 3.2.1 scenarios.

[lsmith77]

does anyone have time to look into fixing the tests?

[oleg-andreyev]

Was planning to help today

[j0k3r]

While it might fix the problem for LiipThemeBundle, won't this generate this error when warming up the cache twigphp/Twig#2281 ?

Instead of this reflexion hack, why not changing the constructor to be compatible with the new FilesystemLoader from TwigBundle and use an extra method call to set the active theme?

In ThemeCompilerPass:

         $twigFilesystemLoaderDefinition = $container->getDefinition('twig.loader.filesystem');
         $twigFilesystemLoaderDefinition->setClass($container->getParameter('liip_theme.filesystem_loader.class'));
-        $twigFilesystemLoaderDefinition->addArgument(new Reference('liip_theme.active_theme'));
+        $twigFilesystemLoaderDefinition->addMethodCall('setActiveTheme', [new Reference('liip_theme.active_theme')]);

In FilesystemLoader:

     /**
      * Constructor.
      *
-     * @param FileLocatorInterface        $locator     A FileLocatorInterface instance
-     * @param TemplateNameParserInterface $parser      A TemplateNameParserInterface instance
-     * @param ActiveTheme                 $activeTheme
+     * @param FileLocatorInterface        $locator  A FileLocatorInterface instance
+     * @param TemplateNameParserInterface $parser   A TemplateNameParserInterface instance
+     * @param string|null                 $rootPath The root path common to all relative paths (null for getcwd())
      */
-    public function __construct(FileLocatorInterface $locator, TemplateNameParserInterface $parser, ActiveTheme $activeTheme = null)
+    public function __construct(FileLocatorInterface $locator, TemplateNameParserInterface $parser, $rootPath = null)
     {
-        parent::__construct(array());
+        parent::__construct(array(), $rootPath);
+
         $this->locator = $locator;
         $this->parser = $parser;
+    }
+
+    /**
+     * Define the active theme
+     *
+     * @param ActiveTheme $activeTheme
+     */
+    public function setActiveTheme(ActiveTheme $activeTheme = null)
+    {
         $this->activeTheme = $activeTheme;
     }

[lsmith77]

Would definitely prefer a solution without reflection .. @j0k3r maybe open your approach as a PR too?

[fotomerchant]

I agree this is a cleaner fix than adding then using reflection in #158.

[lsmith77]

superseded by #159