Allow filters to respond in html

[Kaik]

Allow filters to respond in html

Q	A
Bug fix?	[no]
New feature?	[yes]
BC breaks?	no
Deprecations?	no
Fixed tickets	-
Refs tickets	-
License	MIT
Changelog updated	[no]

[Kaik]

What should I do with SensioLabsInsight — Code quality below expectations ?

[craigh]

What should I do with SensioLabsInsight — Code quality below expectations

nothing

[craigh]

but I'm not sure I'm in favor of this PR. I think all the hook calls apply the |raw filter now, so this shouldn't be needed

[Kaik]

Hmm, I have checked this some time ago and |raw didn't make any changes. I will test it now to confirm.

[Kaik]

Ok this is the call {{ managedPost.get.postText|notifyFilters('dizkus.filter_hooks.post.filter')|raw }} and it does not work, same as {{ managedPost.get.postText|raw|notifyFilters('dizkus.filter_hooks.post.filter')}}
Hooks filter is bbcode filter that returns html instead of [quote].. and without this PR it shows escaped html.

[craigh]

It looks like in the past (1)(2)(3) with Smarty we used |strip_tags|safehtml or at least |safehtml

because hooks potentially provide user content we should probably do the same. So I propose closing this PR and documenting somewhere that hook filters should also be further filtered by the safeHtml filter.

- [x] investigate whether the filter can be implemented internally in the hook filter so it doesn't need to be manually added.

• removal of |raw where in use
  - [ ] if not added internally
• add |safeHtml anywhere hooks are used.
• documentation of requirement

(1) https://github.com/craigh/PostCalendar/blob/master/templates/user/list.tpl#L31
(2) https://github.com/zikula-modules/FAQ/blob/master/templates/user/display.tpl#L4
(3) https://github.com/zikula-modules/News/blob/master/templates/user/preview.tpl#L4

[craigh]

more thoughts:

1. I don't we want to internally implement safeHtml within the notifyFilter filter, because its usage should be optional to the developer.

2. We need to document clearly that when user content is displayed, it should be filtered.

[Kaik]

Hmm I'm not sure if I understand you correctly. There are two things:

1. is_safe => 'html' in Twig HookExtension
2. raw http://stackoverflow.com/questions/8355123/display-a-string-that-contains-html-in-twig-template

First one is deactivating "safe html" feature in notifiFilters method which strips tags by default.
|raw does a similar thing but not for notifyFilters, only for variables so we cannot use it here because tags are already striped somewhere in Twig filters declaration and |raw does not turn it off I guess.

For user content in filter hooks, if tags need to be stripped then this should occur way before notifyFilters call, so hook filters can return output with some additional markup. Best examples are BBCode and BBsmile but I can imagine some kind of censor filter or even words highlighter or something like nl2br.
Exactly similar thing is happening for notifyDisplayHooks.
Maybe order of the filters in twig will do the job

{{ managedPost.get.postText|striptags|notifyFilters('dizkus.filter_hooks.post.filter')}}
I will do some tests today later on this should work if filters are applied in order they are written.
Unless there will be some filters that play with markup like adding classes to divs or something that requires postText to be raw this should work.

Btw correct usage for pages:
This will strip tags befre content is processed by hook filters
{{ content|safeHtml|notifyFilters('pages.filter_hooks.pages.filter') }}
This will strip tags after content is processed by hook filters
{{ content|notifyFilters('pages.filter_hooks.pages.filter')|safeHtml }}
In first case hook filters can respond with markup added by filter while user content is filtered before
Second case filter everything so no html at all...

[craigh]

Hmm I'm not sure if I understand you correctly.

I'm suggesting this is the correct implementation:

{{ content|notifyFilters('pages.filter_hooks.pages.filter')|safeHtml }}

[Kaik]

If is_safe =>'html' will be added to notifyFilters method then this do not really matter as it is developers choice. I think that safeHtml filter should be before notifyFilters so filter hooks can work on already html safe content, while they can inject some html that will not be filtered out.

So this PR, will be merged? or you have other plans?

[Kaik]

And btw I really think we do not understand each other.

I don't we want to internally implement safeHtml within the notifyFilter filter, because its usage should be optional to the developer.

This PR is removing safeHtml filter in notifyFilter by adding 'is_safe' => 'html' option this is disabling twig internall safeHtml filter https://twig.sensiolabs.org/doc/2.x/advanced.html#automatic-escaping
We could go further and use 'pre_escape' => 'html' as well and we could remove safeHtml filter from templates compleatly
$filter = new Twig_Filter('somefilter', 'somefilter', array('pre_escape' => 'html', 'is_safe' => array('html')));

[craigh]

@Kaik please sign the clahub agreement

[craigh]

thank you @Kaik