[RTM] Allow to disable input encoding for a whole dca #708

Toflar · 2017-02-15T08:44:55Z

Here's another feature for Contao 4.4.

For my own modules I want to be able to disable the input encoding all together and output stuff using twig or encode it myself. I have an application which even doesn't output anything at all but manages content that's fetched through an API. I have to set the eval attributes on every field which is a bit tedious. I think it can be very helpful to disable this completely and prepare for the future already in your own bundles/applications :-)

aschempp · 2017-02-15T09:28:24Z

Input would still be encoded, right? I mean script tags and null bytes stripped and other "safety checks". We would need to use the request class instead of the input class if the flag is enabled, right?

I mean I'm totally fine with the feature, but the name is a bit confusing. It would better be something like allowHtmlForAll 😂

Toflar · 2017-02-15T09:30:02Z

It will use Input::postRaw() with this config.

leofeyer · 2017-02-17T14:15:58Z

Input::postRaw() applies the following filters:

Input::preserveBasicEntities()
Input::xssClean()
Input::encodeInsertTags() (FE only)

I guess that's why @aschempp suggested to use the Symfony Request class in this case, which will not alter the input values at all.

Toflar · 2017-02-17T14:18:18Z

Ah, I see. Yeah, so we already have our own use case for #674 😄

aschempp · 2017-02-18T20:14:58Z

You mean the core would apply that if the flag is set? 😂

Toflar · 2017-02-22T08:15:50Z

I don't have another option I guess :) So we need both, a new config and a new eval flag. How would you like to name it? What about disableInputEncoding is not fine? It's pretty much stating what it does :-)

aschempp · 2017-02-24T17:08:01Z

It depends on the implementation. If it means flag all fields with decodeEntities or thelike I'm fine with that. But if it means we use Symfony Request I would prefer something else, like useSymfonyRequest

Toflar · 2017-02-24T17:10:33Z

What do the others think? /cc @contao/developers

discordier · 2017-02-25T08:18:39Z

I'd do some flag on dca level and forward this to the Widget class.
In the widget class, we check the flag and it will then use the raw value from the request.

I think we could combine this with my changes from #575, sadly I still did not come around to shift the remaining methods from Input over to StringUtils so it would be RTM.

ausi · 2017-02-25T10:13:02Z

Looking at this, I thought we must already have such a flag for the password fields in the backend, but it looks like we haven’t. If I change my password in the backend to foobar\0baz I have to login with foobarbaz. So this would be another use case for this feature.

I think the name disableInputEncoding is fine. But I’m not sure if we should use the Symfony request or Input::postUnsafeRaw() instead.

discordier · 2017-02-25T12:32:10Z

Request, definately!

Toflar · 2017-02-27T09:30:06Z

Yeah, request for sure but why should I name the eval option useSymfonyRequest? It's just the regular, unmodified value coming from the current request. Whether it is a Symfony subrequest or not does not matter, it's just representing the current request value. So I'm still all for disableInputEncoding.

bytehead · 2017-02-27T09:32:13Z

I'm fine with disableInputEncoding.

discordier · 2017-02-28T06:58:24Z

So am I.

Toflar · 2017-02-28T07:58:22Z

Okay, but I don't want to rebase all the time. I'll only finish once @leofeyer merged #674.

leofeyer · 2017-03-23T14:45:09Z

As discussed in Mumble on March 23rd, the flag shall be called useRawRequestData and work on table level and on field level.

Toflar · 2017-03-23T15:05:02Z

Note to self: Check password fields (they should not strip null bytes etc.)

…it to either a field or the whole table configuration and fixed encoding on password fields

Toflar · 2017-03-28T11:21:50Z

PR is updated and ready for review.

leofeyer · 2017-03-28T15:31:37Z

src/Resources/contao/library/Contao/Widget.php

@@ -263,6 +265,14 @@ public function __set($strKey, $varValue)
 				$this->strPrefix = $varValue;
 				break;

+			case 'useRawRequestData':


Since you have declared the property as a boolean, we should handle the false case as well, shouldn't we?

Yeah not all attributes do unfortunately. So it's inconsistent already anyway but I added it.

leofeyer · 2017-03-28T15:49:34Z

src/Resources/contao/library/Contao/Widget.php

+				{
+					/** @var Request $request */
+					$request = \System::getContainer()->get('request_stack')->getCurrentRequest();
+					$this->setInputCallback(function() use ($request) {


You could (but don't have to) simplify this:

$this->setInputCallback(function () { return \System::getContainer()->get('request_stack')->getCurrentRequest()->request->get($this->name); });

ausi · 2017-03-28T15:50:09Z

src/Resources/contao/classes/DataContainer.php

@@ -273,6 +273,12 @@ protected function row($strPalette=null)
 			$this->varValue = \StringUtil::insertTagToSrc($this->varValue);
 		}

+		// Use raw request if set globally
+		if (isset($GLOBALS['TL_DCA'][$this->strTable]['config']['useRawRequestData']) && $GLOBALS['TL_DCA'][$this->strTable]['config']['useRawRequestData'] === true)


Should we add a !isset($arrData['eval']['useRawRequestData']) check here too? This would make it possible to use the raw request for the whole DCA but opt-out of it for single fields.

Good idea, added in fd2ef3f.

ausi · 2017-03-28T15:55:08Z

src/Resources/contao/library/Contao/Widget.php

+					/** @var Request $request */
+					$request = \System::getContainer()->get('request_stack')->getCurrentRequest();
+					$this->setInputCallback(function() use ($request) {
+						return $request->request->get($this->name);


It looks like Widget::getPost() supports array syntax like field[foo][bar], do we need this functionality here too?

I don't think so. If you want the raw request data you need to handle the array yourself if you need to, no?

I have no idea where this array syntax is needed, maybe @aschempp can help us here? See Widget.php:781

ausi · 2017-03-28T16:00:08Z

Why did you use setInputCallback()? Couldn’t this be buggy in some cases? e.g.:

$widget->setInputCallback(...);
$widget->useRawRequestData = true;

The second line would overwrite the already set input callback. Couldn’t we just add a check for the useRawRequestData flag in the Widget::getPost() method?

Toflar · 2017-03-28T16:02:44Z

he second line would overwrite the already set input callback.

Exactly. Which is the desired workflow. There can only ever be one responsible function fetching the input. If you handle useRawRequestData within getPost() the setInputCallback() method could result having no effect at all which is very confusing I find.

ausi · 2017-03-28T16:08:21Z

What about this example:

$widget->useRawRequestData = true;
$widget->setInputCallback(null);

This would not use the raw request.

I think this is a confusing public API, if useRawRequestData would just be a simple flag, I could check for this flag in my own input callback too:

$widget->setInputCallback(function() {
	if ($widget->useRawRequestData) {
		...
	}
});

Toflar · 2017-03-28T16:11:36Z

I think this is a confusing public API, if useRawRequestData would just be a simple flag, I could check for this flag in my own input callback too:

Which means that you can change the behavior someone else wanted. In my opinion both ways can be confusing. It's just the way it is.

ausi · 2017-03-28T16:16:11Z

Isn’t this inconsistent? allowHtml => Input::postHtml() and preserveTags => Input::postRaw() are already two flags that Widget::getPost() handles, why should useRawRequestData behave differently than the existing flags?

Toflar · 2017-03-28T16:37:29Z

No idea. Because it's cleaner? :) I changed it.

ausi · 2017-03-29T07:16:41Z

src/Resources/contao/library/Contao/Widget.php

@@ -83,6 +84,7 @@
 * @property string                  $slabel            The submit button label
 * @property boolean                 $preserveTags      Preserve HTML tags
 * @property boolean                 $decodeEntities    Decode HTML entities
+ * @property boolean                 useRawRequestData  Use the raw request data from the Symfony request


Should we add useRawRequestData to the switch-case on line 345 too?

Jop! Added in 31732f9.

aschempp · 2017-03-29T08:48:53Z

src/Resources/contao/library/Contao/User.php

@@ -320,6 +321,8 @@ public function authenticate()
 	 */
 	public function login()
 	{
+		/** @var Request $request */
+		$request = System::getContainer()->get('request_stack')->getCurrentRequest();


This is a behavior change, shouldn't we use getMasterRequest?

I think it's almost always wrong to take the master request. The use cases are only very, very limited. So no: the current request is the way to go.

aschempp · 2017-03-29T08:50:08Z

src/Resources/contao/library/Contao/Widget.php

+		if ($this->useRawRequestData === true)
+		{
+			/** @var Request $request */
+			$request = \System::getContainer()->get('request_stack')->getCurrentRequest();


Same here, I think we should use the master request?

Description ----------- While writing tests for #708 I noticed that the `VarDumper` component isn't actually required by the `core-bundle`, even though `\Contao\Template::dumpTemplateVars()` uses it. This PR adds the dependency and also adds a test for it. _Note:_ this needs to be added to the `composer.json` of the `contao/contao` mono repository as well presumably, but I am not sure what the procedure here is? _Note:_ this requirement is not needed for the `4.4` branch - but the test could be added there. I can provide a separate PR for that. Commits ------- 6ee2945c require symfony/var-dumper 61bd510f simplify dumpTemplateVars test c47b9961 remove unused use statements 2db49377 Fix the Travis build 9dbbb302 Sync the composer.json files

leofeyer assigned Toflar Feb 17, 2017

leofeyer added feature up for discussion labels Feb 17, 2017

leofeyer changed the title ~~Allow to disable input encoding for a whole dca~~ [RFC] Allow to disable input encoding for a whole dca Feb 17, 2017

leofeyer removed the up for discussion label Mar 23, 2017

Allow to fetch the raw post data from the symfony request by setting …

bed0d26

…it to either a field or the whole table configuration and fixed encoding on password fields

leofeyer requested changes Mar 28, 2017

View reviewed changes

leofeyer added this to the 4.4.0 milestone Mar 28, 2017

Handle the false case

68116ce

leofeyer approved these changes Mar 28, 2017

View reviewed changes

ausi reviewed Mar 28, 2017

View reviewed changes

Allow opting out for single fields

fd2ef3f

Moved the handling of the raw request data to the getPost() method

5098055

Toflar changed the title ~~[RFC] Allow to disable input encoding for a whole dca~~ [RTM] Allow to disable input encoding for a whole dca Mar 28, 2017

ausi reviewed Mar 29, 2017

View reviewed changes

Force useRawRequestData param to be a boolean

31732f9

ausi approved these changes Mar 29, 2017

View reviewed changes

aschempp reviewed Mar 29, 2017

View reviewed changes

leofeyer self-assigned this Apr 21, 2017

leofeyer merged commit 750cac6 into contao:develop Apr 21, 2017

Toflar deleted the feature/disable-input-encoding branch April 24, 2017 09:45

ausi mentioned this pull request Sep 14, 2017

[-] gets replaced with  in the template editor #1075

Closed

leofeyer mentioned this pull request Jul 11, 2019

useRawRequestData leads to empty data contao/contao#558

Closed

[RTM] Allow to disable input encoding for a whole dca #708

[RTM] Allow to disable input encoding for a whole dca #708

Conversation

Toflar commented Feb 15, 2017

aschempp commented Feb 15, 2017

Toflar commented Feb 15, 2017

leofeyer commented Feb 17, 2017

Toflar commented Feb 17, 2017 • edited Loading

aschempp commented Feb 18, 2017

Toflar commented Feb 22, 2017

aschempp commented Feb 24, 2017

Toflar commented Feb 24, 2017

discordier commented Feb 25, 2017

ausi commented Feb 25, 2017

discordier commented Feb 25, 2017

Toflar commented Feb 27, 2017

bytehead commented Feb 27, 2017

discordier commented Feb 28, 2017

Toflar commented Feb 28, 2017

leofeyer commented Mar 23, 2017 • edited Loading

Toflar commented Mar 23, 2017

Toflar commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ausi commented Mar 28, 2017

Toflar commented Mar 28, 2017

ausi commented Mar 28, 2017

Toflar commented Mar 28, 2017

ausi commented Mar 28, 2017

Toflar commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toflar commented Feb 17, 2017 •

edited

Loading

leofeyer commented Mar 23, 2017 •

edited

Loading