3.x FormHelper:: Allow to specify the source of injected form field values by ionas · Pull Request #9127 · cakephp/cakephp

ionas · 2016-07-17T13:46:55Z

This PR implements what has been suggested here: #8093 (comment)

In case there is a general consensus that this is a good approach, then I will add:

unit tests
~~doc blocks~~ Done.

I did test this on a simple baked Article edit form:

Example URL: https://cakephp.dev/articles/edit/1?title=foo&body=bar
Example Code:

<?= $this->Form->create($article, ['values' => 'query']) ?>
// or
<?= $this->Form->create($article, ['values' => ['query', 'context']]) ?>

This would take data from query and then from context. It respects order. E.g. this would also work:

<?= $this->Form->create($article, ['values' => ['data', 'query', 'context']]) ?>
// or
<?= $this->Form->create($article, ['values' => ['data', 'query']]) ?>

Note

Initially I wanted to let Contexts handle this. However FormHelper::_addDefaultContextProviders() is being called when the FormHelper is being constructed, not when FormHelper::create() is being called. As far as I can see Construction time would be the only place to pass down data such as $this->request->query or $this->request->data

Concerns

~~Maybe the option flag values should be named valuesSources for the FormHelper::create() options interface, too?~~ Done.

Extensibility

This could also be made configurable when FormHelper is being setup via ::loadHelper().
In case this makes it through I can add support for this.

stickler-ci · 2016-07-17T13:47:18Z

        return [];
    }
+
+    public function getValuesSources()


Missing function doc comment

Missing blank line before return statement

stickler-ci · 2016-07-17T13:52:01Z

+        return $this;
+    }
+
+    public function getSourceValue($fieldname)


Missing function doc comment

stickler-ci · 2016-07-17T14:05:28Z

    }
+
+    /**
+     * T O D O 


Whitespace found at end of line

ADmad · 2016-07-17T14:33:20Z

+            if ($valuesSource === 'context') {
+                return $this->_getContext()->val($fieldname);
+            }
+            if (isset($this->request->{$valuesSource}[$fieldname])) {


You need to use the methods instead of properties else you won't get values for field names like foo.bar.

Yes I considered that. Has that feature been merged yet? (thought it was part of the dot syntax for Entity::get()).

@ADmad So how would I differentiate between a legitimate null/false and the method just not finding anything within the path?

1176: * When reading values you will get null for keys/values that do not exist.
http://api.cakephp.org/3.3/source-class-Cake.Network.Request.html#1166-1203

Due to data() being a getter and setter I cannot even patch it to take a flag to be strict and through exceptions when values are not being found.

@markstory any ideas? New methods like getData(), setData(), getQuery(), setQuery()? where the last is $options and there you could disable throwing of exceptions?

https://github.com/cakephp/cakephp/blob/master/src/View/Form/EntityContext.php#L216

So how would I differentiate between a legitimate null/false and the method just not finding anything within the path?

You don't need to since post data and query string can only have string values.

For unmodified $_POST and $_GET, yes - for $this->request->data and $this->request->query, not necessarily.

It fails for instance if you do $this->request->data = $this->Articles->get($id) and it does fill some fields with null which you then overwrite via query sourceValues e.g. <?= $this->Form->create(null, ['values' => ['data', 'query']]) ?> - not that I advertise such code.

…ing there is never null within $this->data->post[]/query[]

ADmad · 2016-07-17T17:30:56Z

            'encoding' => strtolower(Configure::read('App.encoding')),
            'templates' => null,
            'idPrefix' => null,
+            'valuesSources' => [],


This should default to ['data', 'context'].

Right my mind was broken when I removed it! 'Context' alone would do it. Both or just context?

Yes, just ['context'] would be enough.

ionas · 2016-07-17T20:44:29Z

Please let me know if anything is missing/wrong/off. I will probably find time the next weekend to add unit tests so that this PR becomes merge-able.

@michaelze let me know if this would be sufficient for your use cases because it is an alternative implementation to your original PR.

Edit: I'll take a look at why tests are failing ASAP - If you have hints/pointers. Let me know.

markstory · 2016-07-18T02:05:43Z

+     * Gets a single field value from the sources available.
+     *
+     * @param string $fieldname The fieldname to fetch the value for.
+     * @return string Field value derived from sources.


Should be string|null as the bottom of the method can return a null implicitly.

markstory · 2016-07-18T02:07:00Z

@ionas The tests are failing because valuesSource is being turned into an HTML attribute from what I can see.

…) are being run

markstory · 2016-08-21T23:41:54Z

You'll need to resolve the merge conflicts.

markstory · 2016-08-21T23:45:30Z

+
+    /**
+     * Tests the different input rendering values based on sources values switching while supplying
+     * an entity (base context) and multiple sources (durch as data, query)


Typo in such.

inoas · 2016-08-22T10:22:26Z

I have resolved the merge conflict by adding support for defaults and schemaDefaults for valuesSources.

inoas · 2016-08-25T03:06:24Z

Anything else? I don't want it to get out of sync with HEAD/master again ;).

inoas · 2016-08-25T20:39:46Z

+                return $this->request->{$valuesSource}($fieldname);
+            }
+        }
+        if ($options['default'] !== null && $options['default'] !== false) {


Is false a good condition?

Why do you need to exclude false? Boolean fields can have a default value of false.

markstory · 2016-08-26T02:13:28Z

+        }
+        if ($options['schemaDefault'] !== null && $options['schemaDefault'] !== false) {
+            return $options['schemaDefault'];
+        }


Could tests be added for this behavior?

Done. I did also remove the false check and hardened the not null test via isset().

…added tests for that

… context source

stickler-ci · 2016-08-26T19:21:10Z

+                return $this->request->{$valuesSource}($fieldname);
+            }
+        }
+        return null;


Missing blank line before return statement

markstory · 2016-08-26T22:30:23Z

+        $this->Form->setValuesSources(['query']);
+        $result = $this->Form->getSourceValue('title');
+        $expected = '';
+        $this->assertEquals($expected, $result);


Why don't the schema defaults apply when the source is 'query'?

if you want to apply the schema defaults you need to do:

$this->Form->setValuesSources('context'); // The default // or $this->Form->setValuesSources(['query', 'context']); // or $this->Form->setValuesSources(['context', 'query']);

Without allowing the context, you will get nothing out of it. At least that was the goal.
And yes, usually you want the context, and you can have it.

If the context is required to get default values, why is this code here? EntityContext:val() already handles providing schema defaults.

Because you can supply 'schemaDefaults' as an option to FormHelper::create() but not utilise the EntityContext?

You say I should remove:

if (isset($options['schemaDefault'])) { return $options['schemaDefault']; }

?

Its not in the source anymore anyway - I removed it in the last commit @ above line.
So what is missing from your point of view? What do you want me to change?

Why don't the schema defaults apply when the source is 'query'?

@markstory Because only context instance knows about the schema defaults. So just setting sources to ['query'] wouldn't would apply schema defaults but setting it to ['query', 'context'] would.

@ADmad Ok that makes sense to me. The duplicate code I was concerned about has been removed.

dereuromark · 2016-08-27T00:37:27Z

So is this going into master or the 3.next outlined on the right sidebar?

markstory · 2016-08-27T02:03:22Z

@dereuromark I'll merge it into 3.next. I'm a little gun-shy on adding potentially disruptive features into master given how rough 3.3.x has been so far.

inoas · 2016-08-27T10:44:44Z

While I don't look forward to updating it again in terms of merge conflicts I am 👍 on going more with semver on non-tiny-features. Updating docs. Let me know if I need to do anything else in before 3.next merge.

Ref: cakephp/cakephp#9127

allow to specify the source of injected form field values

521c42f

stickler-ci reviewed Jul 17, 2016
View reviewed changes

stickler-ci

e9e5277

Missing blank line before return statement

stickler-ci reviewed Jul 17, 2016
View reviewed changes

placeholder docblocks

f41fdbc

stickler-ci reviewed Jul 17, 2016
View reviewed changes

Comment thread src/View/Helper/FormHelper.php Outdated

}

/**

* T O D O

Copy link
Copy Markdown

Contributor

stickler-ci Jul 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace found at end of line

make stickler as quite as possible for the time being :E

9b49d5a

ADmad reviewed Jul 17, 2016
View reviewed changes

Jonas Hartmann added 2 commits July 17, 2016 16:17

use methods instead of array access to allow dot notation while assum…

4bbd72e

…ing there is never null within $this->data->post[]/query[]

remove default values for vanilla behaivor

48bd343

ADmad reviewed Jul 17, 2016
View reviewed changes

Jonas Hartmann added 2 commits July 17, 2016 20:39

declare instance variable, add docblocks

b37f531

remove empty line

87bcbac

markstory added this to the 3.2.13 milestone Jul 18, 2016

markstory added enhancement helpers labels Jul 18, 2016

markstory reviewed Jul 18, 2016
View reviewed changes

Jonas Hartmann added 2 commits July 18, 2016 19:23

remove consumed option, declare correct return types

c31b83e

set context as default valueSource even before constructor or create(…

6bcd8a1

…) are being run

inoas mentioned this pull request Aug 21, 2016

FormHelper sourcesValues feature docs cakephp/docs#4265

Closed

markstory reviewed Aug 21, 2016
View reviewed changes

Jonas Hartmann added 2 commits August 22, 2016 10:19

master merge

0775337

typo durch

cb979eb

ionas force-pushed the formhelper-values branch from 6b41d1b to cb979eb Compare August 22, 2016 10:20

markstory modified the milestones: 3.3.2, 3.3.3 Aug 22, 2016

markstory self-assigned this Aug 25, 2016

inoas reviewed Aug 25, 2016
View reviewed changes

markstory modified the milestones: 3.4.0, 3.3.3 Aug 26, 2016

markstory reviewed Aug 26, 2016
View reviewed changes

Jonas Hartmann added 2 commits August 26, 2016 18:37

fixed default/schemaDefault null-case scenario (isset not ==! null), …

4eecbfe

…added tests for that

made source context pass through to next source if no value found for…

f7d1ddd

… context source

stickler-ci reviewed Aug 26, 2016
View reviewed changes

blank line in before closing return statement

e311da6

markstory reviewed Aug 26, 2016
View reviewed changes

markstory changed the base branch from master to 3.next August 27, 2016 14:32

markstory pushed a commit to cakephp/docs that referenced this pull request Aug 30, 2016

FormHelper sourcesValues feature docs

0e76b4e

Ref: cakephp/cakephp#9127

markstory merged commit 6a509da into cakephp:3.next Aug 30, 2016

markstory mentioned this pull request Aug 30, 2016

Formhelper source rename #9371

Merged

Conversation

ionas commented Jul 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Note

Concerns

Extensibility

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ionas Jul 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ionas Jul 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ionas Jul 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ionas commented Jul 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markstory commented Jul 18, 2016

Uh oh!

markstory commented Aug 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inoas commented Aug 22, 2016

Uh oh!

inoas commented Aug 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inoas Aug 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inoas Aug 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ionas commented Jul 17, 2016 •

edited

Loading

ionas Jul 17, 2016 •

edited

Loading

ionas Jul 17, 2016 •

edited

Loading

ionas Jul 17, 2016 •

edited

Loading

ionas commented Jul 17, 2016 •

edited

Loading

inoas Aug 27, 2016 •

edited

Loading

inoas Aug 27, 2016 •

edited

Loading