[RTM] Allow to manually pass a value to any widget #674

Toflar · 2017-01-17T15:06:53Z

I suppose it will probably still take some time until our widgets become obsolete so here's something we should have had ever since there was Contao :-P

leofeyer · 2017-01-17T15:11:11Z

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

@@ -381,11 +381,11 @@ public function __get($strKey)

 			case 'value':
 				// Encrypt the value
-				if ($this->arrConfiguration['encrypt'])
+				if (isset($this->arrConfiguration['encrypt']) && true === $this->arrConfiguration['encrypt'])


This change – although it is not wrong – is not technically required, is it?

It somehow is because if I left it as is, I would have needed to adjust my unit test and suppress "undefined index encrypt" notices and I figured instead of fixing the unit test I might as well just fix the source of the problem :D

Isn't isset($this->arrConfiguration['encrypt']) also true for $this->arrConfiguration['encrypt'] = false;?
Therefore this seems to be required IMO.

Edit: I was talking about the additional true check only, not the whole change which is needed for sake of "undefined index" notices of course.

leofeyer · 2017-01-17T15:11:16Z

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

 				{
 					return \Encryption::encrypt($this->varValue);
 				}
-				elseif ($this->varValue == '')
+				elseif ($this->varValue === '')


This change – although it is not wrong – is not technically required, is it?

It is. $this->varValue == '') would be true if $this->varValue is null ;-) A case I would have never thought of without unit tests ;-)

leofeyer · 2017-01-17T15:13:03Z

tests/Contao/WidgetTest.php

+        /** @var Widget $widget */
+        $widget->validate(null, true);
+        $this->assertNull($widget->value);
+    }


There should be another test for ->validate(null, false).

Right, there was even a bug which I fixed with the unit test covering that case :-D See df901df.

Toflar · 2017-01-17T15:25:19Z

Failing tests not related.

aschempp

This will significantly change the signature of existing methods, and that will cause issues with existing widgets. Why not simply add a new method for this case which will also not need the ugly $treatNullAsValue hack?

Toflar · 2017-01-17T16:16:29Z

Thougt of this too but it doesn't change the signature to be clear. Both values are optional.

aschempp · 2017-01-17T16:30:38Z

If my widget has currently implemented the validate() method, PHP will raise an error because my widget does no longer follow the parent. Same issue we're having in isotope/core#1753

A method like validateCustom($value) would not need a second parameter because calling that method would always mean null should be treated as value.

Toflar · 2017-01-17T16:40:42Z

Okay, proposal for a method name? Not sure if setValue() is too simple? :D

aschempp · 2017-01-17T19:20:24Z

setValue does not imply that it's validated. How about validateValue()?

leofeyer · 2017-01-17T19:24:57Z

How about overrideValue()? We could also make it setPost() if the change is supposed to be permanent.

Toflar · 2017-01-18T08:39:08Z

setValue does not imply that it's validated. How about validateValue()?

It does not necessarily need to be validated imho. So setValue() is actually correct.
Basically it should just take a custom value instead of searching in the post values when you call validate(). A different method like validateValue() would not be a good idea as you could not set the value in one component and let it validate in another.

How about overrideValue()? We could also make it setPost() if the change is supposed to be permanent.

Excuse me? That would be completely wrong and would make this PR completely useless? I can already set the post value now but that's exactly what's wrong and I want to omit?

I think $widget->setValue()->validate(). Would be correct, I'm adjusting the PR so you can see what I mean.

Toflar · 2017-01-18T08:57:21Z

Okay, next round. Just one more comment on this version now:

Yes I did think of adding only one method instead of two but I want to cover the null value case. The only other idea was to make getPost() public because then you could do $widget->setCustomValue($widget->getPost($widget->name)) to set it back to the original behavior. But this would again change the method signature of a method so I guess we're safer the way I built it now.

leofeyer · 2017-01-18T10:55:04Z

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

 	 * Validate the user input and set the value
 	 */
 	public function validate()
 	{
-		$varValue = $this->validator($this->getPost($this->strName));
+		$varValue = ($this->blnCustomValueSet) ? $this->varCustomValue : $this->getPost($this->strName);


The parentheses around $this->blnCustomValueSet can be omitted. Also, I'd prefer blnHasCustomValue.

aschempp · 2017-01-18T11:12:13Z

This does not make sense to me at all. If you want to set a value, simply use $widget->value = …. The __set() method will take care of assigning it to the variable. If you want validation, that's why I suggested validateValue().

Toflar · 2017-01-18T11:13:57Z

If you want validation, that's why I suggested validateValue().

And again, this cannot work as I pointed out before. The method cannot be named differently because all the current code calls validate() now.

leofeyer · 2017-01-18T13:39:21Z

The current implementation dose make sense, just the naming is not good yet. How about setInput() or setInputValue()?

aschempp · 2017-01-18T14:46:47Z

So @Toflar explained the issue to me. I didn't realize he's using the regular form generator and therefore cannot modify the flow. I just came up with another idea that I like much more than those two ugly properties.

I suggest to add a setInputCallback() method that accepts a callable. If the callable is set, it will be called instead of getPost. This will allow for more flexibility as well, and it is easier to understand.

Getting input from input value is fairly easy.

$widget->setInputCallback(function ($strName) {
    return \Input::get($strName);
});

so is getting it from prepared data

$widget->setInputCallback([$myclass, 'getValue']);

// MyClass
function getValue($strName) {
    return $this->data[$strName];
}

The Widget::validate method then should look like this:

$varValue = (null !== $this->inputCallback ? call_user_func($this->inputCallback, $strName) : $this->getPost($this->strName));

leofeyer · 2017-01-18T15:12:42Z

Fine with me, too.

…g from post values

Toflar · 2017-01-24T11:21:31Z

PR and tests updated accordingly. RTM.

aschempp

We're currently not using fluent interfaces anywhere, but not really a problem.

discordier · 2017-01-24T14:24:07Z

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

 	 * Validate the user input and set the value
 	 */
 	public function validate()
 	{
-		$varValue = $this->validator($this->getPost($this->strName));
+        $varValue = (null !== $this->inputCallback ? call_user_func($this->inputCallback) : $this->getPost($this->strName));


Shouldn't this rather be:

- $varValue = $this->validator($this->getPost($this->strName)); + $varValue = (is_callable($this->inputCallback) ? call_user_func($this->inputCallback) : $this->getPost($this->strName));

IMO this will prevent errors when wrong values get passed or a class is not loadable or the like.
What do you think?

There's a callable type hint, you cannot set anything else than callable or null ;)

Of course I can, I can inject ['SomeUnloadableClass', 'someMethod'] where is_callable will try to autoload and return false where your solution will trigger a "Fatal error: class not found".

leofeyer · 2017-03-16T14:24:55Z

What about @discordier's unanswered comment? I cannot merge the PR until it has been addressed.

Toflar · 2017-03-20T12:34:10Z

It has been addressed. I wrote there is a type hint for callable. You cannot inject an array...dunno what @discordier was writing. The PR is RTM to me.

aschempp · 2017-03-20T13:42:05Z

@Toflar callable does allow an array, maybe you were looking for \Closure?

Toflar · 2017-03-20T13:44:24Z

Yeah we could do this but I don't understand why it would be relevant in any way. If you provide a wrong callable you did something wrong? An error is better than falling back to the Input::post() stuff?

aschempp · 2017-03-20T13:52:19Z

I agree, if a callable is set we should try to call it and not simply ignore…

leofeyer · 2017-03-21T09:31:37Z

I also think that we should support [$obj, 'method'], which can be passed to call_user_func().

* Make Symfony 3.2 the minimum requirement (see contao#630). * Set the encryption key from the kernel secret by default (see contao#660). * Stop using the deprecated QuestionHelper::setInputStream() method. * Update Dropzone to version 4. * Prefer the caret operator over the tilde operator in the composer.json file. * Add the contao.root_dir parameter (see contao#662). * Update the change log. * Stop using the contao-components/all meta package. * Update the README.md file. * Deprecate the contao:version command (see contao#668). * Update the installation path. * Auto-select the active page in the quick navigation/link module (see contao/core#8587). * Look up the form class and allow to choose the type (see contao/core#8527). * Add PHP Toolbox support (see contao#565). * Remove the arrow brackets in the book navigation template (see contao/core#8607). * Add a bottom padding to the buttons layer. * Add the contao.web_dir parameter (see contao/installation-bundle#40). * Fix the tests. * Match security firewall based on request scope (see contao#677). * Fix an issue found by Scrutinizer. * Use the contao.web_dir parameter in the Combiner (see contao#679). * Fix the tests. * Add stripRootDir() method to System class (see contao#683). * Add the contao.image.target_dir parameter (see contao#684). * The ContaoCoreExtension::overwriteImageTargetDir() is not deprecated. * Support custom backend routes (see contao#512). * Use the scope matcher instead of checking the request attribute (see contao#688). * Replace every occurrence of $contaoFramework with $framework. * Fix an issue found by Scrutinizer. * Fix deprecations in unit tests (see contao#687). * Added a DBAL field type for UUIDs (see contao#415). * Support importing form field options from a CSV field (see contao#444). * Fix the coding style and the unit tests. * Add the Doctrine field type in the config.yml file. doctrine: dbal: types: binary_string: class: "Contao\\CoreBundle\\Doctrine\\DBAL\\Types\\BinaryStringType" commented: true * Add a basic unit test for the BackendCsvImportController class. * Update the change log. * Fix rebuilding the search index (see contao#689). * Also handle „no origin“ and „empty origin“ in the CORS provider. * Remove an unused use statement. * Remove the security.yml file and update the README.md file. * Improve the e-mail extraction in the text element (thanks to Martin Auswöger). * Rename the Test namespace to Tests. * Update the composer.json file. * Update the .php_cs file. * Raise the minimum PHP version to 5.6 (see contao#701). * Support using objects in callback arrays (see contao#699). * Use try-finally blocks to close all output buffers when downloading a file (see contao#714). * Fix the coding style. * Only prefix an all numeric alias when standardizing (see contao#707). * Adjust the test namespaces. * Allow to manually pass a value to any widget (see contao#674). * Add a change log entry and fix the tests. * Disable the picker buttons if the main window does not show a picker. * Use the file manager instead of the file picker. * Use the site structure instead of the page picker. * Always show the selected nodes. * Add the menu builder.

leofeyer requested changes Jan 17, 2017

View reviewed changes

leofeyer added the feature label Jan 17, 2017

leofeyer assigned Toflar Jan 17, 2017

leofeyer added this to the 4.4.0 milestone Jan 17, 2017

aschempp suggested changes Jan 17, 2017

View reviewed changes

leofeyer reviewed Jan 18, 2017

View reviewed changes

Allow to manually pass a value to any widget instead of always pullin…

df87269

…g from post values

Toflar changed the title ~~Allow to manually pass a value to any widget~~ [RTM] Allow to manually pass a value to any widget Jan 24, 2017

aschempp approved these changes Jan 24, 2017

View reviewed changes

discordier reviewed Jan 24, 2017

View reviewed changes

leofeyer force-pushed the develop branch 2 times, most recently from c8f2f0d to 09536d6 Compare January 27, 2017 16:49

Toflar mentioned this pull request Feb 17, 2017

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

Merged

leofeyer approved these changes Mar 23, 2017

View reviewed changes

leofeyer self-assigned this Mar 23, 2017

leofeyer added 4 commits March 28, 2017 10:56

Prevent a possible BC break if a non-boolean value is used.

4e3cc22

Use is_callable() instead of !== null.

4087bc1

Use tabs to indent lines.

14a3d18

Fix the coding style.

b126752

leofeyer merged commit 16f7445 into contao:develop Mar 28, 2017

Toflar deleted the widget-pass-custom-value branch March 28, 2017 09:17

agoat mentioned this pull request Apr 10, 2017

Feature/dca picker (#46) agoat/contao-core-bundle#47

Merged

discordier mentioned this pull request Mar 13, 2018

Feature Widget::setInputCallback() is incomplete. #1427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTM] Allow to manually pass a value to any widget #674

[RTM] Allow to manually pass a value to any widget #674

Toflar commented Jan 17, 2017

leofeyer Jan 17, 2017

Toflar Jan 17, 2017

discordier Jan 24, 2017 •

edited

Loading

leofeyer Jan 17, 2017

Toflar Jan 17, 2017

leofeyer Jan 17, 2017

Toflar Jan 17, 2017

Toflar commented Jan 17, 2017

aschempp left a comment

Toflar commented Jan 17, 2017 via email

aschempp commented Jan 17, 2017

Toflar commented Jan 17, 2017 via email

aschempp commented Jan 17, 2017

leofeyer commented Jan 17, 2017

Toflar commented Jan 18, 2017

Toflar commented Jan 18, 2017

leofeyer Jan 18, 2017

aschempp commented Jan 18, 2017

Toflar commented Jan 18, 2017

leofeyer commented Jan 18, 2017

aschempp commented Jan 18, 2017 •

edited

Loading

leofeyer commented Jan 18, 2017

Toflar commented Jan 24, 2017

aschempp left a comment

discordier Jan 24, 2017

Toflar Jan 24, 2017

discordier Jan 25, 2017

leofeyer commented Mar 16, 2017

Toflar commented Mar 20, 2017

aschempp commented Mar 20, 2017

Toflar commented Mar 20, 2017

aschempp commented Mar 20, 2017

leofeyer commented Mar 21, 2017

[RTM] Allow to manually pass a value to any widget #674

[RTM] Allow to manually pass a value to any widget #674

Conversation

Toflar commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

discordier Jan 24, 2017 • edited Loading

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 Jan 17, 2017

aschempp left a comment

Choose a reason for hiding this comment

Toflar commented Jan 17, 2017 via email

aschempp commented Jan 17, 2017

Toflar commented Jan 17, 2017 via email

aschempp commented Jan 17, 2017

leofeyer commented Jan 17, 2017

Toflar commented Jan 18, 2017

Toflar commented Jan 18, 2017

Choose a reason for hiding this comment

aschempp commented Jan 18, 2017

Toflar commented Jan 18, 2017

leofeyer commented Jan 18, 2017

aschempp commented Jan 18, 2017 • edited Loading

leofeyer commented Jan 18, 2017

Toflar commented Jan 24, 2017

aschempp left a 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

leofeyer commented Mar 16, 2017

Toflar commented Mar 20, 2017

aschempp commented Mar 20, 2017

Toflar commented Mar 20, 2017

aschempp commented Mar 20, 2017

leofeyer commented Mar 21, 2017

discordier Jan 24, 2017 •

edited

Loading

aschempp commented Jan 18, 2017 •

edited

Loading