Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

add errorClass option to div options. [FormHelper] #1079

Closed
wants to merge 1 commit into from

5 participants

@intuxicated

no comment , lol

@intuxicated intuxicated Update lib/Cake/View/Helper/FormHelper.php
add errorClass option to div options.
b9c471d
@markstory
Owner

I'm not a fan of this. Its a conventions over configuration framework for a reason. There is such a thing as too much configuration.

@markstory
Owner

New features need tests, and need to not target master as well. No new features get merged into master.

@markstory markstory closed this
@intuxicated

thanks for fast replay ,
actually i don't think this is new feature , its something like missed feature or something like that.
i don't know why but many things in this framework forced to developers, this PR is just one example of it.

https://github.com/cakephp/cakephp/blob/master/lib/Cake/View/Helper/FormHelper.php#L2638
is another example.

i have to design my UI as framework likes it. its not even fear because i use framework for server side not client side and you just telling me GFYS :+1: :)

about master branch , sorry , my bad.

btw , its just 2 simple line code and i think you can test it by reading.
anyway.

@ADmad
Collaborator

i have to design my UI as framework likes it. its not even fear because i use framework for server side not client side and you just telling me GFYS :)

No one is forcing you to use the FormHelper as it is. You can easily extend it and override required function. In 2.3 FormHelper::input() has been split up into multiple functions to make overriding a particular step of the element generation process easier. CakePHP has "convention over configuration" philosophy. You are free to ignore conventions in most cases but that would just mean more work for you.

btw , its just 2 simple line code and i think you can test it by reading.

Your statement tells me you don't seem to fully understand what unit testing is and its benefits.

@lorenzo
Owner

Despite being in the wrong branch an having no tests, do anyone think this is a good idea?

@markstory
Owner

Testing by reading is not maintainable in the long term, and not how CakePHP operates. Reading the code is fine for now but doesn't ensure the feature will stay working in the future. While the code changes seem small they are much larger when the greater context of documentation and tests are considered. Both of those aspects are not present in this pull request. Features like this slowly increase the complexity, requiring more tests, more documentation and more maintenance overall.

I can understand wanting to control every aspect of every classname in generated HTML. Perhaps if you need that level of control FormHelper is not the ideal solution, and perhaps subclassing or re-implementing it would be a better option for you.

I also understand that this option is one of the few that is missing. In my opinion FormHelper like PaginatorHelper has far too many options many of which should probably not exist, but we can't remove them at this point in time. We can however stem the ever growing complexity by not adding configuration where its not absolutely required.

@intuxicated

@ADmad , thanks for comment

Your statement tells me you don't seem to fully understand what unit testing is and its benefits.

i know, and i fully understand what unit testing is. but seems like you didn't read my commit.

@lorenzo , i should say sorry to everyone who read this PR about wrong branch ? do you think this is a good idea ?

@markstory , thanks for comment

Testing by reading is not maintainable in the long term, and not how CakePHP ...

i know i know. i was sleepy and little angry , forgive me about "btw ..." section.

I can understand wanting to control every aspect of every classname in generated HTML. Perhaps if you need that level of control FormHelper is not the ideal solution, and perhaps subclassing or re-implementing it would be a better option for you.
@ADmad
No one is forcing you to use the FormHelper as it is. You can easily extend it and override required function. In 2.3 FormHelper::input() has been split up into multiple functions to make overriding a particular step of the element generation process easier. CakePHP has "convention over configuration" philosophy. You are free to ignore conventions in most cases but that would just mean more work for you.

re implementing its good idea if i want to generate a custom element. such as jstree element or etc. when i use FormHelper::input i want to generate a element with my options. and i see when i have error in my form i have element with unexpected class. this is good idea. so i'm going to read the documentation to customize error class with what i want, or remove that if i don't want it. but there is no document, and there is no option. i read the code and i see

$divOptions = $this->addClass($divOptions, 'error');

this is exactly what hard code means. and I'm trying to fix this. my commit does not affect to inputs without errorClass option, and that still return error class for who don't use this option and i did not remove anything. so you don't have to worry about this.

you telling me extend FormHelper class to remove something that shouldn't be there at first place. it means more work as @ADmad said, and its not rapid. as a developer i wont re implement a php class to customize a css class. i prefer to make change in FormHelper to generate exact output that i want to. you can reject this PR but you can't tell me i'm wrong about this change.
i just become a developer for 7 years and i know i have to learn many things. so correct me if I'm wrong.

I also understand that this option is one of the few that is missing. In my opinion FormHelper like PaginatorHelper has far too many options many of which should probably not exist, but we can't remove them at this point in time. We can however stem the ever growing complexity by not adding configuration where its not absolutely required.

thanks for understanding.

Note : this commit is buggy, i fix it immediately after sending PR but i didn't send another commit because i though no one is interesting of it.

@leandrocr

Hey, this should be implemented in CakePHP main code!

i tried, they don't like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 22, 2013
  1. @intuxicated

    Update lib/Cake/View/Helper/FormHelper.php

    intuxicated authored
    add errorClass option to div options.
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 1 deletion.
  1. +2 −1  lib/Cake/View/Helper/FormHelper.php
View
3  lib/Cake/View/Helper/FormHelper.php
@@ -1149,7 +1149,8 @@ public function input($fieldName, $options = array()) {
if ($type != 'hidden' && $error !== false) {
$errMsg = $this->error($fieldName, $error);
if ($errMsg) {
- $divOptions = $this->addClass($divOptions, 'error');
+ $errorClass = (isset($divOptions['errorClass'])) ? $divOptions['errorClass'] : 'error';
+ $divOptions = $this->addClass($divOptions, $errorClass);
$out['error'] = $errMsg;
}
}
Something went wrong with that request. Please try again.