Skip to content

3.0 form input #2891

Merged
merged 26 commits into from Feb 25, 2014

3 participants

@lorenzo
CakePHP member
lorenzo commented Feb 23, 2014

Part of #2267. This makes FormHelper::input() work again. I removed a few options along the way:

  • before, between and after placeholders are gone, you need to change the template to introduce those.
  • Not possible to pass formatting options to the error container using array keys, you need to change the template instead.

Changed some of the magic type selection:

  • If you manually specify the type, the helper will still attach the magic form attributes such as maxLength. This was not possible in 2.x
  • Refactored the introspection functions, now they look cleaner and more focused.

Some problems that I found:

  • The helper is making too many assumptions about the templates it uses, it basically is trying itself to the default templates. An example of this is that I had to unset($options['type']) in both file and checkbox, which are two of the fields that have a special template.
  • Another example where the helper is too coupled to its default templates is that it is not possible, so far, to nest the input field inside the label. This is required for some bootstrap field types and is one of the options for inline forms in foundation.
  • Specifying a table context was super difficult to figure out, I'd like to have a way of making it easier/clearer.
@lorenzo

@markstory I'm trying to make input work, but I really cannot figure out how the context should work. No matter what I pass here is somehow wrong. Can you lend me a hand here? This is the 3.0-form-input branch

CakePHP member

I can try and fix up some of the tests tonight when I get to my computer.

CakePHP member

I already made a bunch of them work, not sure if that was the correct way, though :P

CakePHP member

So far I have been passing this->article in as it uses the array context.

@ADmad
CakePHP member
ADmad commented Feb 24, 2014

Another example where the helper is too coupled to its default templates is that it is not possible, so far, to nest the input field inside the label.

It is possible to nest input inside label as shown in this test for radio input. Though modifying the label template would mean it would affect all labels, which would be a problem if we need the change only for particular input types.

@lorenzo
CakePHP member
lorenzo commented Feb 24, 2014

One question I had is, should we always prepend the root table name for the automatic id generation? I think that using the input name only will generate several id collisions

@ADmad
CakePHP member
ADmad commented Feb 24, 2014

Without prefixing indeed there would be high probability of id collisions. So either a prefix should be used based on context passed or there should be an option to pass a custom prefix to Form->create() as I had suggested earlier.

@ADmad ADmad commented on an outdated diff Feb 24, 2014
src/View/Helper/FormHelper.php
];
/**
- * Copies the validationErrors variable from the View object into this instance
+ * Construct the widgets and binds the defult context providers
@ADmad
CakePHP member
ADmad added a note Feb 24, 2014

defult => default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ADmad ADmad and 1 other commented on an outdated diff Feb 24, 2014
src/View/Helper/FormHelper.php
@@ -845,156 +848,149 @@ public function inputs($fields = null, $blacklist = null, $options = array()) {
* @link http://book.cakephp.org/2.0/en/core-libraries/helpers/form.html#creating-form-elements
*/
public function input($fieldName, $options = array()) {
- $this->setEntity($fieldName);
- $options = $this->_parseOptions($options);
-
- $divOptions = $this->_divOptions($options);
- unset($options['div']);
-
- if ($options['type'] === 'radio' && isset($options['options'])) {
- $radioOptions = (array)$options['options'];
- unset($options['options']);
- }
+ $options += [
+ 'type' => null,
+ 'label' => null,
+ 'error' => null,
+ 'selected' => null,
@ADmad
CakePHP member
ADmad added a note Feb 24, 2014

I can't find where is the selected key used.

@markstory
CakePHP member
markstory added a note Feb 24, 2014

Selected should be value/val now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ADmad
CakePHP member
ADmad commented Feb 24, 2014

One inconsistency that's exists in 2.x which i would like to get rid of is, if you use dateTime() directly by default it has 'empty' => true but when using input() date time selects by default have 'empty' => false.

@ADmad ADmad commented on the diff Feb 24, 2014
src/View/Helper/FormHelper.php
}
- if ($type === 'radio' && isset($out['between'])) {
- $options['between'] = $out['between'];
- $out['between'] = null;
- }
- $out['input'] = $this->_getInput(compact('type', 'fieldName', 'options', 'radioOptions', 'selected', 'dateFormat', 'timeFormat'));
+ $groupTemplate = $options['type'] === 'checkbox' ? 'checkboxFormGroup' : 'formGroup';
+ $input = $this->_getInput($fieldName, $options);
@ADmad
CakePHP member
ADmad added a note Feb 24, 2014

The input tag should probably be generated before the label to allow nesting the input inside the label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on an outdated diff Feb 24, 2014
src/View/Helper/FormHelper.php
}
- if (in_array($options['type'], array('radio', 'select'))) {
- $options = $this->_optionsOptions($options);
+ $options = $this->_magicOptions($fieldName, $options, $needsMagicType);
+ return $options;
+ }
+
+/**
+ * Returns the input type that was guessed for the provided fieldName,
+ * based on the internal type it is associated too, its name and the
+ * variales that can be foudn in the view template
@markstory
CakePHP member
markstory added a note Feb 24, 2014

Typos on this line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on the diff Feb 24, 2014
src/View/Helper/FormHelper.php
*
+ * @param string $fieldName the name of the field to find options for
* @param array $options
* @return array
@markstory
CakePHP member
markstory added a note Feb 24, 2014

I was wondering if we want to keep this convention. I know it has confused people in the past. But it does make the simple cases even simpler.

@lorenzo
CakePHP member
lorenzo added a note Feb 24, 2014

Maybe it is a good idea to kiss it good bye, it does not offer anything useful for real

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on the diff Feb 24, 2014
src/View/Helper/FormHelper.php
}
}
- if (preg_match('/_id$/', $fieldKey) && $options['type'] !== 'hidden') {
- $options['type'] = 'select';
- }
+ // Missing HABTM
+ //...
@markstory
CakePHP member
markstory added a note Feb 24, 2014

We might be able to use the field name of _ids to infer habtm here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on the diff Feb 24, 2014
src/View/Helper/FormHelper.php
@@ -1790,7 +1689,7 @@ public function select($fieldName, $options = [], $attributes = []) {
);
$hidden = $this->hidden($fieldName, $hiddenAttributes);
}
- unset($attributes['hiddenField']);
+ unset($attributes['hiddenField'], $attributes['type']);
return $hidden . $this->widget('select', $attributes);
@markstory
CakePHP member
markstory added a note Feb 24, 2014

Perhaps the widget classes should exclude the type option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lorenzo
CakePHP member
lorenzo commented Feb 25, 2014

I have fixed the stuff that could be immediately fixed, there is still plenty of test cases to update for input(), do you think we should merge this and work on top of it?

@ADmad
CakePHP member
ADmad commented Feb 25, 2014

:+1:

@markstory
CakePHP member

@lorenzo I think that is quite reasonable. It also makes it easier for us to work as a group on fixing the remaining tests/issues.

@markstory markstory merged commit 91b78c0 into 3.0 Feb 25, 2014

1 check passed

Details default The Travis CI build passed
@markstory markstory deleted the 3.0-form-input branch Feb 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.