Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter block truncating content #576

Closed
ElChupacabra26 opened this issue Nov 6, 2015 · 16 comments
Closed

Filter block truncating content #576

ElChupacabra26 opened this issue Nov 6, 2015 · 16 comments
Labels

Comments

@ElChupacabra26
Copy link

Filter blocks with template info are getting chopped off at TOKEN_VARIABLE_START. What I'm trying to do is pass a template string to my filter, so I can do some processing before calling renderString() on the block contents. Something like this:

{% filter foo %}
    <div class="singleVal">
        Your val is: {{someVal}}
    </div>
{% endfilter %}
syncFilters.foo = function foo (fooContent) {
    var output = '';
    var vals = require('config').someVals || ['default'];

    for (var i = 0; i < vals.length; i++) {
        output += nunjucks.renderString(fooContent, {someVal: vals[i]});
    }

    return output;
};

Is this the expected behavior and if so, is there another way of passing template data into a filter?

@carljm
Copy link
Contributor

carljm commented Nov 6, 2015

I would expect the content within the {% filter %} tag to be parsed and processed normally before being passed to the filter, so {{ someVal }} would be replaced by its value.

If you didn't want that, you'd wrap the contents in a {% raw %} tag.

If it's really just being cut off entirely, that seems like a bug that should be fixed.

@fabien
Copy link
Contributor

fabien commented Nov 8, 2015

I actually have the following test-case fail after updating nunjucks today, so I suspect there's something wrong indeed.

it('should handle tag: filter', function(next) {
    var tmpl = '{% filter replace("Bar", "TEST") %}\n';
    tmpl += 'Filtered: {{foo}}\n'; 
    tmpl += '{% endfilter %}'; 
    application.renderString(tmpl, { foo: 'FooBar' }, function(err, res) {
        should.not.exist(err);
        res.should.containEql('Filtered: FooTEST'); // failed output: 'Filtered: '
        next();
    });
});

@ElChupacabra26
Copy link
Author

@carljm I actually tried wrapping my content in the raw tag as well which ended up giving me an empty string. It seems it is being truncated entirely. For example, Your val is: {{someVal}}... more text passes the content up to the {{ token and chops the rest off.

@fabien Under parseFilterStatement, nunjucks is passing body.children[0]... to the nodelist init, but I'm not sure if there's a reason it's being done that way... Didn't really try to dig any deeper honestly.

@carljm
Copy link
Contributor

carljm commented Nov 10, 2015

@fabien So you're saying this used to work, but broke at some point? Any chance you could bisect the commit that broke it?

@fabien
Copy link
Contributor

fabien commented Nov 10, 2015

@carljm turns out I had implemented my own filter tag, and now that it became a native nunjucks tag, it wasn't used anymore. My own tag implementation did work as expected (interpolating the content of the block before filtering).

@carljm
Copy link
Contributor

carljm commented Nov 10, 2015

@fabien Ah! Well then, any chance you'd be willing to share your implementation with the community by submitting it as PR to fix the broken built-in version? :-)

@fabien
Copy link
Contributor

fabien commented Nov 10, 2015

I'll look into it - IIRC I had my own filter for it. Then again, it might have been a fork of this one:
https://github.com/SamyPesse/nunjucks-filter

@vkbansal
Copy link

I too am facing similar issue. My code is as follows

// Not working. Returns empty string
{% filter replace('\n', '') %}
    {% block content %}{% endblock %}
{% endfilter %}

// Working
{% block content %}{% endblock %}

jrehwaldt pushed a commit to jrehwaldt/nunjucks that referenced this issue Jan 21, 2016
… The additional tests focus on template constructs within the filter block (`{% filter ... %}...{% endfilter %}` as introduced by mozilla#254):

1. variable evaluation inside filter block
2. block declaration inside filter block

Both test are added to the list of `it('should handle filter blocks', ...`, which seems to be the project's convention for grouping tests by feature. Nonetheless, the two added test cases fail in different ways:
1. The variable expression test fails due to the discarding of any but the first token. Its output is unexpected.
2. The block declaration test fails due to a reference to undefined. It throws an error.

Both issues are caused by the following line https://github.com/mozilla/nunjucks/blob/d4b3a07603b5a5c8adacb0c54aca53df8c931c73/src/parser.js#L1032. It can easily be seen that the given code comment is only true for text-only filter blocks.
@jrehwaldt
Copy link
Contributor

Any hints how this can be fixed? I added test cases for @vkbansal's (also mine) as well as @fabien's usecase in #647. Any ideas I had on how to fix this failed so far due to my incomplete knowledge of the parser/compiler.

@carljm
Copy link
Contributor

carljm commented Jan 21, 2016

@jrehwaldt That's an awesome contribution, thanks! It definitely looks like you've narrowed things down to the problematic assumption in the code. I think it's likely that with some study I (or someone else familiar with the internals) could give some useful guidance on how to approach the fix. Unfortunately I don't have time for that focused study today, or probably this week, but I'll put this at the top of my nunjucks list to get to when I can. Maybe someone else will be able to provide some useful feedback in the meantime.

Thanks again!

@ElChupacabra26
Copy link
Author

@carljm I remember diving into the internals a few months back when I first ran into this problem and found a possible culprit, but wasn't familiar enough with the Nunj internals to dive any deeper. Not sure if it's the actual culprit, but see my second comment:

Under parseFilterStatement, nunjucks is passing body.children[0]... to the nodelist init

Maybe that'll give you something to go off of when you find some time to look into this issue.

@carljm
Copy link
Contributor

carljm commented Jan 22, 2016

@ElChupacabra26 Yeah, thanks for reminding me of that comment. That's actually the same observation @jrehwaldt makes in the linked tests PR.

@ElChupacabra26
Copy link
Author

@carljm Ah yes, I see that now. Thanks

@carljm carljm added the 3.x label Feb 18, 2016
@carljm carljm closed this as completed in 647f9d5 Mar 14, 2016
carljm added a commit that referenced this issue Mar 14, 2016
* t576:
  Add test verifying that block-set can wrap a block.
  Fix filter block tag (fixes #576).
  Refactor this block of test assertions into separate tests.
  Adds *failing* tests illustrating both issues mentioned in #576. The additional tests focus on template constructs within the filter block (`{% filter ... %}...{% endfilter %}` as introduced by #254): 1. variable evaluation inside filter block 2. block declaration inside filter block
@carljm
Copy link
Contributor

carljm commented Mar 14, 2016

Fixed! Thanks @jrehwaldt for the failing test, and everyone for the investigation and pointers.

carljm added a commit that referenced this issue Mar 14, 2016
* t576:
  Add test verifying that block-set can wrap a block.
  Fix filter block tag (fixes #576).
  Refactor this block of test assertions into separate tests.
  Adds *failing* tests illustrating both issues mentioned in #576. The additional tests focus on template constructs within the filter block (`{% filter ... %}...{% endfilter %}` as introduced by #254): 1. variable evaluation inside filter block 2. block declaration inside filter block
@jrehwaldt
Copy link
Contributor

Thank you. Works perfectly.

@ElChupacabra26
Copy link
Author

Thanks for the fix @carljm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants