Removal of the Assetic\Extension\Twig\TwigFormulaLoader class about assetic HOT 10 CLOSED

Good point @LukeTowers, this came dangerously close to ending on a "It's OK, I fixed it" note for future viewers.

Here's the code we're using (and don't believe that first comment, it's been edited since then):

<?php

/**
 * This is a copy of the original class which was removed from the
 * assetic-php/assetic project.
 *
 * See https://github.com/assetic-php/assetic/issues/15
 */

namespace DoW\Slim\Assetic\Extension\Twig;

use Assetic\Contracts\Factory\Loader\FormulaLoaderInterface;
use Assetic\Contracts\Factory\Resource\ResourceInterface;
use Assetic\Extension\Twig\AsseticNode;
use Psr\Log\LoggerInterface;
use Slim\Assetic\Extension\Twig\AsseticFilterNode;
use Twig\Environment;
use Twig\Node\ModuleNode;
use Twig\Node\Node;
use Twig\Source;

/**
 * Loads asset formulae from Twig templates.
 *
 * @author Kris Wallsmith <[email protected]>
 */
class TwigFormulaLoader implements FormulaLoaderInterface
{
    private $twig;
    private $logger;

    public function __construct(Environment $twig, LoggerInterface $logger = null)
    {
        $this->twig = $twig;
        $this->logger = $logger;
    }

    public function load(ResourceInterface $resource)
    {
        try {
            $tokens = $this->twig->tokenize(new Source($resource->getContent(), (string) $resource));
            $nodes  = $this->twig->parse($tokens);
        } catch (\Exception $e) {
            if ($this->logger) {
                $this->logger->error(sprintf('The template "%s" contains an error: %s', $resource, $e->getMessage()));
            }

            return array();
        }

        return $this->loadNode($nodes);
    }

    /**
     * Loads assets from the supplied node.
     *
     * @param Node $node
     *
     * @return array An array of asset formulae indexed by name
     */
    private function loadNode(Node $node)
    {
        $formulae = array();

        if ($node instanceof AsseticNode) {
            $formulae[$node->getAttribute('name')] = array(
                $node->getAttribute('inputs'),
                $node->getAttribute('filters'),
                array(
                    'output'  => $node->getAttribute('asset')->getTargetPath(),
                    'name'    => $node->getAttribute('name'),
                    'debug'   => $node->getAttribute('debug'),
                    'combine' => $node->getAttribute('combine'),
                    'vars'    => $node->getAttribute('vars'),
                ),
            );
        }

        foreach ($node as $child) {
            if ($child instanceof Node) {
                $formulae += $this->loadNode($child);
            }
        }

        if ($node->hasAttribute('embedded_templates')) {
            foreach ($node->getAttribute('embedded_templates') as $child) {
                $formulae += $this->loadNode($child);
            }
        }

        return $formulae;
    }
}

As a bonus, here's the diff and commit message which produced the above code (so you can regenerate the true original if you so desire):

    maint: fix assets generation
    
    Assetic's TwigFormulaLoader was referencing AsseticFilterNode which doesn't exist
    anymore in the codebase. This is used when passing functions to the AsseticExtension
    which we are not doing, so it is safe to remove support for this functionality.

diff --git a/src/lib/Slim/Assetic/Extension/Twig/TwigFormulaLoader.php b/src/lib/Slim/Assetic/Extension/Twig/TwigFormulaLoader.php
index 1f61f0b9..afbd8cfe 100644
--- a/src/lib/Slim/Assetic/Extension/Twig/TwigFormulaLoader.php
+++ b/src/lib/Slim/Assetic/Extension/Twig/TwigFormulaLoader.php
@@ -11,9 +11,12 @@ namespace DoW\Slim\Assetic\Extension\Twig;
 
 use Assetic\Contracts\Factory\Loader\FormulaLoaderInterface;
 use Assetic\Contracts\Factory\Resource\ResourceInterface;
+use Assetic\Extension\Twig\AsseticNode;
 use Psr\Log\LoggerInterface;
+use Slim\Assetic\Extension\Twig\AsseticFilterNode;
 use Twig\Environment;
 use Twig\Node\ModuleNode;
+use Twig\Node\Node;
 use Twig\Source;
 
 /**
@@ -55,7 +58,7 @@ class TwigFormulaLoader implements FormulaLoaderInterface
      *
      * @return array An array of asset formulae indexed by name
      */
-    private function loadNode(ModuleNode $node)
+    private function loadNode(Node $node)
     {
         $formulae = array();
 
@@ -71,25 +74,6 @@ class TwigFormulaLoader implements FormulaLoaderInterface
                     'vars'    => $node->getAttribute('vars'),
                 ),
             );
-        } elseif ($node instanceof AsseticFilterNode) {
-            $name = $node->getAttribute('name');
-
-            $arguments = array();
-            foreach ($node->getNode('arguments') as $argument) {
-                $arguments[] = eval('return '.$this->twig->compile($argument).';');
-            }
-
-            $invoker = $this->twig->getExtension('Assetic\Extension\Twig\AsseticExtension')->getFilterInvoker($name);
-
-            $inputs  = isset($arguments[0]) ? (array) $arguments[0] : array();
-            $filters = $invoker->getFilters();
-            $options = array_replace($invoker->getOptions(), isset($arguments[1]) ? $arguments[1] : array());
-
-            if (!isset($options['name'])) {
-                $options['name'] = $invoker->getFactory()->generateAssetName($inputs, $filters, $options);
-            }
-
-            $formulae[$options['name']] = array($inputs, $filters, $options);
         }
 
         foreach ($node as $child) {

Hope this helps anyone from the future.

from assetic.

Hi @jaxwilko,

sorry for the late answer, I have now caught up on my other tasks and can focus on this issue again.

So as I reimplemented the TwigFormulaLoader on our end, the "Unknown class" error disappeared and everything seemed to work fine. Unfortunately it actually didn't, as a member of the team realised this week: no file is output and this is probably because, as you clearly mentioned above, we don't encounter any AsseticNode or AsseticFilterNode at all during tree traversal. Running the tests didn't dig up any problem because I still had the previously generated files on my machine; it's only when we needed to run our test suite in a newly built container that we realised something was wrong.

We are in the process of fixing this in our codebase. I will update here when we're done, see if it's worth putting this code back into assetic, or if it's an end-user problem.

from assetic.

Hi @laurentbardin, from memory I can't remember why this was removed although it may have been due to incompatibility with other 2.0 changes, I'll take and see if we can reimplement those classes without causing any issues

from assetic.

@laurentbardin thanks!

I think the reason why I removed it was due to the deprecation of Twig_Node_Expression_Function.

There is Twig\Node\Expression\FunctionExpression which should replace it. I'll see what can be done :)

from assetic.

Hi @laurentbardin,

I've reimplmented the functionality here: 37a3e95 which is on the branch twig-formula-loader.

The only issue I can see is to do with the formula function test which doesn't return the correct response.

tests/Assetic/Test/Extension/Twig/TwigFormulaLoaderTest.php

Which results in:

From what I can tell the code gets to here:

src/Assetic/Extension/Twig/TwigFormulaLoader.php

But we never encounter a node of AsseticNode or AsseticFilterNode which means that we iterate through the tree and end up creating no formula and returning an empty array.

Unfortunately I don't have any more time to look into this today, feel free to take a look and raise a PR if you find anything, if not I'll try to take a look tomorrow.

from assetic.

Hi @jaxwilko, thank you for the quick answer. As I mentioned, I am reimplementing the class on our side as we speak (while also updating it for Twig 2.11, of course), so should I bump into any compatibility issue I will update you here as well.

from assetic.

@laurentbardin Ah, thanks for feeding back! Would be really interesting to find the issue with the nodes not being found, might be worth looking at Twig changes that have happened between 1.x and 2.0 Assetic.

Maybe we're not implementing the nodes properly or something.

I'll try to take a look when I get some free time but feel free to raise a PR if you get it working :)

from assetic.

@laurentbardin any updates on this?

from assetic.

Hi @LukeTowers,

sorry for not keeping you up to date with this. This issue looks like it can be put to rest without any regret: between now and my last message, we have updated our dependencies and everything looks fine now between twig 2.14 and assetic/framework. We also have an in-house implementation of TwigFormulaLoader to fit our needs. It doesn't do much, but I can copy-paste it here if you're curious.

from assetic.

Sure @laurentbardin, even if we don't end up implementing it, could be useful for future viewers of this issue

from assetic.