Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

3.0 form input #2891

Merged
merged 26 commits into from

3 participants

José Lorenzo Rodríguez ADmad Mark Story
José Lorenzo Rodríguez
Owner

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.
José Lorenzo Rodríguez

@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

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

Owner

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

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

ADmad
Collaborator

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.

José Lorenzo Rodríguez
Owner

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
Collaborator

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.

src/View/Helper/FormHelper.php
((8 lines not shown))
];
/**
- * Copies the validationErrors variable from the View object into this instance
+ * Construct the widgets and binds the defult context providers
ADmad Collaborator
ADmad added a note

defult => default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Collaborator
ADmad added a note

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

Mark Story Owner

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
Collaborator

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
src/View/Helper/FormHelper.php
((69 lines not shown))
}
- 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 Collaborator
ADmad added a note

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
src/View/Helper/FormHelper.php
((171 lines not shown))
}
- 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
Mark Story Owner

Typos on this line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Mark Story markstory commented on the diff
src/View/Helper/FormHelper.php
((232 lines not shown))
*
+ * @param string $fieldName the name of the field to find options for
* @param array $options
* @return array
Mark Story Owner

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.

José Lorenzo Rodríguez Owner
lorenzo added a note

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
Mark Story markstory commented on the diff
src/View/Helper/FormHelper.php
((65 lines not shown))
}
}
- if (preg_match('/_id$/', $fieldKey) && $options['type'] !== 'hidden') {
- $options['type'] = 'select';
- }
+ // Missing HABTM
+ //...
Mark Story Owner

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
Mark Story markstory commented on the diff
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);
Mark Story Owner

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
José Lorenzo Rodríguez
Owner

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
Collaborator

:+1:

Mark Story
Owner

@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.

Mark Story markstory merged commit 91b78c0 into from
Mark Story markstory deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 23, 2014
  1. José Lorenzo Rodríguez
  2. José Lorenzo Rodríguez
  3. José Lorenzo Rodríguez
  4. José Lorenzo Rodríguez
  5. José Lorenzo Rodríguez
  6. José Lorenzo Rodríguez

    Template for file input already has the type in the string, removing the

    lorenzo authored
    option so that it does not appear twice
  7. José Lorenzo Rodríguez

    More tests passing

    lorenzo authored
  8. José Lorenzo Rodríguez
  9. José Lorenzo Rodríguez

    Fixing another test

    lorenzo authored
  10. José Lorenzo Rodríguez
  11. José Lorenzo Rodríguez

    Completing doc blocks

    lorenzo authored
  12. José Lorenzo Rodríguez

    Fixing failing test case

    lorenzo authored
  13. José Lorenzo Rodríguez
  14. José Lorenzo Rodríguez
  15. José Lorenzo Rodríguez

    Fixing another test case

    lorenzo authored
  16. José Lorenzo Rodríguez

    Fixing a couple other tests

    lorenzo authored
Commits on Feb 24, 2014
  1. José Lorenzo Rodríguez
  2. José Lorenzo Rodríguez
  3. José Lorenzo Rodríguez
  4. José Lorenzo Rodríguez
  5. José Lorenzo Rodríguez
  6. José Lorenzo Rodríguez

    inputDefaults is gone now

    lorenzo authored
  7. José Lorenzo Rodríguez

    Fixing another test

    lorenzo authored
  8. José Lorenzo Rodríguez
  9. José Lorenzo Rodríguez
  10. José Lorenzo Rodríguez

    Fixed typo

    lorenzo authored
Something went wrong with that request. Please try again.