form_button() and form_submit() no longer set an unnecessary name attrib... #1506

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

tobz-nz commented Jun 19, 2012

...ute by default.

Contributor

alexbilbie commented Jun 19, 2012

Why is the name field unnecessary?

You won't be able to pick up the values of them in a post array if they're not being set...

Contributor

narfbg commented Jun 19, 2012

I've had situations where I don't want to receive those, so I can imagine why this would be useful. It's more typical when GET is used though.

But completely removing the attribute isn't the proper solution - it should be made optional.

Contributor

alexbilbie commented Jun 19, 2012

But surely if you don't want them you can just ignore them

Contributor

narfbg commented Jun 19, 2012

That's why I said it's more common when method="get" is used - it appears in the URL then and that's when it gets annoying. If POST is used, it's just a few more bytes of bandwidth and indeed can be ignored. :)

tobz-nz commented Jun 19, 2012

This is just having no name by default.
99% of the time you don't want a name on submit buttons.
By removing it from the default array you no longer HAVE to have a name, but if you do want one, just add it to the attributes array when creating the button.
Best of both worlds.

Contributor

narfbg commented Jun 19, 2012

I see your point, but 99% of the time I DO want a name on submit buttons. You might be the other way around, but that doesn't necessarily mean that it's the case for most developers. Keep it the old way, just make it optional - don't put it only if it's an empty string.

tobz-nz commented Jun 19, 2012

That's what I've done.

Currently, the name is not optional, but forced. If you do not enter a name, an empty name attr is created anyway.

With this change if you do not enter a name, then no name is added which makes much more sense.

T.

Toby Evans
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Wednesday, 20 June 2012 at 12:08 AM, Andrey Andreev wrote:

I see your point, but 99% of the time I DO want a name on submit buttons. You might be the other way around, but that doesn't necessarily mean that it's the case for most developers. Keep it the old way, just make it optional - don't put it only if it's an empty string.


Reply to this email directly or view it on GitHub:
EllisLab#1506 (comment)

Contributor

narfbg commented Jun 19, 2012

I get that, but you're completely removing the logic for when $data is not an array. I agree that it should not be forced, but if it's a non-empty string - it should still be set as the name attribute:

! is_array($data) && ! empty($data)

tobz-nz commented Jun 19, 2012

Ah, right I see what you mean. So maybe something like:

...
$defaults = array('type' => 'button');
if ( ! is_array($data) && ! empty($data)) $data = array('name'=>$data);
...

tobz-nz commented Jun 19, 2012

Actually I guess it should just use what the other functions use:

if ( ! is_array($data))
{
    $data = array('name' => $data);
}

tobz-nz commented Jun 19, 2012

no wait, that what is was before...
gah, early morning brain...

system/helpers/form_helper.php
@@ -445,7 +445,8 @@ function form_radio($data = '', $value = '', $checked = FALSE, $extra = '')
*/
function form_submit($data = '', $value = '', $extra = '')
{
- $defaults = array('type' => 'submit', 'name' => ( ! is_array($data) ? $data : ''), 'value' => $value);
+ $defaults = array('type' => 'submit', 'value' => $value);
+ if ( ! is_array($data) && ! empty($data)) $data = array('name'=>$data);
@narfbg

narfbg Jun 19, 2012

Contributor

Um, needs to comply with the style guide (http://codeigniter.com/user_guide/general/styleguide.html):

if ( ! empty($data) && ! is_array($data))
{
    $data = array('name' => $data);
}

... also add empty lines after line 448 and before 449/450, for better readability.
Same for form_button() and the changelog entry should probably be altered as well.

tobz-nz commented Jun 19, 2012

Updated to comply with the style guide.

+
+ if ( ! is_array($data) && ! empty($data))
+ {
+ $data = array('name'=>$data);
@narfbg

narfbg Jun 20, 2012

Contributor

Spaces around =>, as in my example. (form_button() as well)

+ {
+ $data = array('name'=>$data);
+ }
+
if (is_array($data) && isset($data['content']))
@narfbg

narfbg Jun 20, 2012

Contributor

At a second look, with the new stuff - this one should be made an elseif.

@@ -51,6 +51,7 @@ Release Date: Not Released
- Helpers
+ - ``form_button()`` and ``form_submit()`` no longer set an unnecessary name attribute by default.
@narfbg

narfbg Jun 20, 2012

Contributor

... no longer set the 'name' attribute if it's empty.

Contributor

narfbg commented Jun 20, 2012

Seems to be causing issues with form_submit():

<h4>A PHP Error was encountered</h4>

<p>Severity: Notice</p>
<p>Message:  Undefined index: name</p>
<p>Filename: helpers/form_helper.php</p>
<p>Line Number: 929</p>
<p>Backtrace: </p>
        <p style="margin-left:10px">
        File: /var/www/citest/application/controllers/welcome.php<br />
        Line: 35<br />
        Function: form_submit           </p>

        <p style="margin-left:10px">
        File: /var/www/citest/index.php<br />
        Line: 269<br />
        Function: require_once          </p>
</p>

tobz-nz commented Jun 20, 2012

ah... ok how about setting a random name in _parse_form_attributes with uniqid() if no name is set?

if ($key === 'value')
            {
                if ( ! isset($default['name']))
                {
                    $default['name'] = uniqid();
                }
Contributor

narfbg commented Jun 20, 2012

That's kind of pointless. There's two reasons for not setting the name attribute - saving a few bytes of traffic or getting rid of the &name=blah in your query string. If it's not that - why care if it's empty or a random string?

tobz-nz commented Jun 20, 2012

The random string won't get added to the final output, its only used for the form_prep() function.
The point is that if your saving/processing the whole post/get array, you don't want to have to remove some fields from it manually. And you don't have invalid markup with empty name attributes.

Contributor

narfbg commented Jun 21, 2012

Okay, I see now that it won't get into the final output. But it doesn't need to use uniqid() - make it an empty string instead.

tobz-nz commented Jun 22, 2012

An empty string would render the $prepped_fields array in form_prep() useless. Maybe thats ok?
That was the only reason for the uniqid().

Contributor

narfbg commented Jun 22, 2012

Why/how would it become useless?

tobz-nz commented Jul 2, 2012

Because the $default['name'] is used as the key for $prepped_fields array.
If it was an empty string it'd not do what its supposed to do.

@narfbg narfbg closed this in 60b9714 Oct 25, 2012

Contributor

narfbg commented Oct 25, 2012

Manually implemented due to the multiple changes to the form helpers since.

nonchip pushed a commit to nonchip/CodeIgniter that referenced this pull request Jun 29, 2013

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