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

BUGFIX: Support the parser halt fluid token #1450

Merged
merged 1 commit into from Nov 23, 2018

Conversation

Projects
None yet
6 participants
@vertexvaar
Copy link
Contributor

vertexvaar commented Nov 19, 2018

This adds the missing template pre-processor to the Neos.FluidAdaptor RenderingContext.
Actually, it is not missing but gets removed upon setting the template pre-processors.

Fixes #1449

BUGFIX: Support the parser halt fluid token
By simply adding the original template pre-processor to the context
this problem will be fixed.

fixes: #1449

@kdambekalns kdambekalns changed the base branch from 4.0 to 4.3 Nov 23, 2018

@kdambekalns

This comment has been minimized.

Copy link
Member

kdambekalns commented Nov 23, 2018

Thanks!

I changed the target branch, since 4.1 to 4.2 are in security-fix-only mode by now.

Here is the remaining oridignal PR comment:


How to verify it

Trying to reproduce will produce the expected behavior (see the issue)

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 23, 2018

I have to admit that I have no clue how those templateProcessors work exactly.
But looks reasonable, especially if it fixes the issue.

From the Neos standpoint this is a new feature though.. I can't judge the "breakability" of this one

@albe

albe approved these changes Nov 23, 2018

Copy link
Member

albe left a comment

Looks good and I don't think this is breaking, since it just enables a feature of Fluid that we left out. Thanks also for looking into the version matching!

@vertexvaar

This comment has been minimized.

Copy link
Contributor Author

vertexvaar commented Nov 23, 2018

I changed the target branch, since 4.1 to 4.2 are in security-fix-only mode by now.

@kdambekalns i always get these wrong, sorry 😅

I have to admit that I have no clue how those templateProcessors work exactly.

@bwaidelich They should rather be called TemplatePreProcessors, because that's what they actually do. They do stuff like CDATA section protection (in Neos.FluidAdaptor. Fluid standalone would drop them), check registered namespaces as xmlns declaration and/or old style namespace declaration in (curly) braces and remove them from the rendering as well as the html tag if it has the data-namespace-typo3-fluid attribute. In the case of the PassthroughSourceModifierTemplateProcessor it will throw an exception to prevent fluid parsing at all. All in all no witchcraft at all 😉

Thanks also for looking into the version matching!

@albe You're welcome.

All together thanks for the approvals (and upcoming merge) 👍

@jonnitto jonnitto merged commit e761859 into neos:4.3 Nov 23, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kitsunet

This comment has been minimized.

Copy link
Member

kitsunet commented Nov 23, 2018

We should probably make the list of processors a configuration at some point.

@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Nov 23, 2018

We should probably make the list of processors a configuration at some point.

An important feature of Fluid was always that a template was self-contained (i.e. you don't need any context to know how the template works). Pre processors are a nice feature but IMO they should not be configurable. At least not too easily :) otherwise you could break a totally unrelated part by changing the configuration.

@kitsunet

This comment has been minimized.

Copy link
Member

kitsunet commented Nov 23, 2018

Good point, well then less work 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.