starting refactor Form->input #875

Merged
merged 2 commits into from Dec 10, 2012

Conversation

Projects
None yet
3 participants
Member

ceeram commented Sep 28, 2012

Here is a start on splitting up Form::input() into smaller pieces, probably can improve more, just like to get some feedback on what i have so far.

Also docblocks need to be completed on new methods.

You like where this is going so far?

@@ -5966,7 +5966,7 @@ public function testTextAreaWithStupidCharacters() {
'label' => 'Current Text', 'value' => "GREAT®", 'rows' => '15', 'cols' => '75'
));
$expected = array(
- 'div' => array('class' => 'input text'),
+ 'div' => array('class' => 'input textarea'),
@markstory

markstory Sep 29, 2012

Owner

Won't this have the possibility of breaking css on some apps? We could do input text textarea to be more fully backwards compatible.

@ceeram

ceeram Sep 29, 2012

Member

I thought this was wrong in the tests, if the input has 'type' => 'textarea' it will get this class on the div. In this test it isnt specified, but it makes it type textarea magically, because rows and cols are specified, i dont understand why it shouldnt get the textarea class if it was set magically

@markstory

markstory Sep 29, 2012

Owner

I agree that the textarea class makes sense, my only worry is removing the text class.

@ceeram

ceeram Sep 29, 2012

Member

This is the current behavior:

    $result = $this->Form->input('Post.content', array('rows' => '15', 'cols' => '75'));
    debug($result);
    '<div class="input text"><label for="PostContent">Content</label><textarea name="data[Post][content]" rows="15" cols="75" id="PostContent"></textarea></div>'

    $result = $this->Form->input('Post.content', array('rows' => '15', 'cols' => '75', 'type' => 'textarea'));
    debug($result);
    '<div class="input textarea"><label for="PostContent">Content</label><textarea name="data[Post][content]" rows="15" cols="75" id="PostContent"></textarea></div>'

Which i would think is not correct in the first place, or am i wrong here? i think its a bug that it doesnt set correct class when type is magically defined by presence of cols/rows

@markstory

markstory Sep 29, 2012

Owner

Oh that is a bit silly.

+ * @return type
+ */
+ protected function _getInput($args) {
+ extract($args);
@markstory

markstory Sep 29, 2012

Owner

I'm a bit more leery of extract() these days. It makes it hard to figure out what exactly the local scope looks like.

@ceeram

ceeram Sep 29, 2012

Member

ill remove the extracts and write out the parameters, although it means having many arguments, which i dnot like either personally

@markstory

markstory Sep 29, 2012

Owner

Or alternatively use $args['fieldName']. You could make local variables for the most commonly used indexes as well.

Owner

markstory commented Sep 29, 2012

I like the direction this is headed. More smaller methods makes the code easier to read and write tests for later down the road. It also opens the ability for classes extending FormHelper to override specific bits instead of input() in its entirety.

Contributor

bar commented Dec 8, 2012

👍 as well

Owner

markstory commented Dec 8, 2012

@ceeram If you rebase this, I don't see any reason why it couldn't be merged in.

Member

ceeram commented Dec 8, 2012

I will add some descriptions to the new protected methods and rebase it to merge.

ceeram added a commit that referenced this pull request Dec 10, 2012

@ceeram ceeram merged commit 505cf3d into cakephp:2.3 Dec 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment