Form_helper by Portaflex · Pull Request #281 · codeigniter4/CodeIgniter4

Portaflex · 2016-09-30T16:01:52Z

Form helper revisited.

Portaflex · 2016-09-30T16:10:41Z

There is one issue with this helper: For the HTML5 output element we would need a class instead of a helper. This is the code to be generated:

`
0

100 +

`

We have to pass the "oninput" attr to form_open() function.

lonnieezell

FYI - There's no need to delete a PR and submit a new one. You lose discussion history when you do that. Just add new commits to the branch and it will show up in the same PR.

This is still missing the HTML inputs I asked for last time, like email, number, date, etc.

Also, you need to provide tests for all of the fields. It looks like you brought over the existing tests from CI3, but then never added any more.

lonnieezell · 2016-10-03T03:19:56Z

system/Helpers/form_helper.php

+     * @param	mixed
+     * @return	string
+     */
+    function _attributes_to_string($attributes): string


I believe we can remove this method and use the stringify_attributes that is in the Common file....

Please double-check, but at a glance, they appear the same.

lonnieezell · 2016-10-03T03:21:37Z

tests/system/Helpers/FormHelperText.php

@@ -0,0 +1,201 @@
+<?php namespace CodeIgniter\Helpers;
+
+class FormHelperTest extends \CIUnitTestCase


The filename is incorrect here. You have FormHelperText, instead of FormHelperTest. Did you run this locally before providing a PR?

Yeah, name changed and local test passed. ;-) I'll try to add some more test.

lonnieezell · 2016-10-03T03:23:00Z

system/Helpers/form_helper.php

+
+// ------------------------------------------------------------------------
+
+if ( ! function_exists('_parse_form_attributes'))


Please remove the CI3 style naming where functions start with underscores.

lonnieezell · 2016-10-03T03:25:00Z

system/Helpers/form_helper.php

+    {
+        $value = $_POST[$field] ?? $default;
+
+        return ($html_escape) ? html_escape($value) : $value;


No function named html_escape exists in the framework. You should be using the esc() method instead. Which will also require you ensure it's loaded....

lonnieezell · 2016-10-03T03:31:48Z

system/Helpers/form_helper.php

+
+if ( ! function_exists('form_output'))
+{
+    function form_output()


This should be possible as a simple field. While other fields need to be attached to it, usually through JS, the output field itself seems to be a standalone field if I understand this one correctly.

No, no. It's not a standalone field. Look at the code:

`

0 100 + =

`
You have to declare the oninput html event in the form opening label. So, you need to have the oninput operation set before calling form_open().

alright, let's simplify then and scrap this one also. Not sure how often people actually use it anyway...

Right. I would say nobody uses it.

lonnieezell · 2016-10-03T03:33:24Z

system/Helpers/form_helper.php

+
+if ( ! function_exists('form_keygen'))
+{
+    function form_keygen()


This element doesn't seem to be used by any browsers at the moment, so scrap this method.

Ok, scrapped.

BTW I closed one PR and opened a new one because some files from other branches "slided" into this one. I didn't know how to remove them. :-(

Makes sense. No worries, just trying to keep things orderly where possible. :)

Portaflex · 2016-10-04T21:17:25Z

We have this:

HTML5 input types:

color
date
datetime
datetime-local
email
month
number
range
search
tel
time
url
week

HTML5 input attributes:

autocomplete
autofocus
form
formaction
formenctype
formmethod
formnovalidate
formtarget
height and width
list
min and max
multiple
pattern (regexp)
placeholder
required
step

And almost all of them can be passed as attributes to form_input() as $data or $extra. A few attributes don't go as a key = value pair, e.g. "disabled" and "readonly".

So which of them are we gonna put in?.

lonnieezell · 2016-10-05T03:13:04Z

And almost all of them can be passed as attributes to form_input() as $data or $extra. A few attributes don't go as a key = value pair, e.g. "disabled" and "readonly".

So which of them are we gonna put in?.

Ideally all of the input types should be included. Another option, and a simpler and more future-proof version, would be to adjust the form_input() method to allow you to specify the type.

As for the attributes, allow them all. No sense restricting. I'm not sure if it would be best to do special checks here, or modify stringify_attributes to check for numeric $keys and create them as single. I think the only place that method is used currently is a couple of methods in the url helper, which would face the same issue, so it's probably safe to do that.

lonnieezell · 2016-11-03T03:36:44Z

Updated and merged in #298

Portaflex added 3 commits September 30, 2016 18:00

Form_helper

6d278ac

Form_helper

bd49ade

Portaflex

b494ffe

lonnieezell requested changes Oct 3, 2016

View reviewed changes

Form_helper

0df139e

lonnieezell mentioned this pull request Nov 3, 2016

Form helper #298

Merged

lonnieezell closed this Nov 3, 2016

Portaflex deleted the Form_helper branch November 8, 2016 23:17

		@@ -0,0 +1,201 @@
		<?php namespace CodeIgniter\Helpers;

		class FormHelperTest extends \CIUnitTestCase


		// ------------------------------------------------------------------------

		if ( ! function_exists('_parse_form_attributes'))

Conversation

Portaflex commented Sep 30, 2016

Uh oh!

Portaflex commented Sep 30, 2016

` 0 100 +

Uh oh!

lonnieezell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Portaflex Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Portaflex commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonnieezell commented Oct 5, 2016

Uh oh!

lonnieezell commented Nov 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`
0

100 +

Portaflex Oct 4, 2016 •

edited

Loading

Portaflex commented Oct 4, 2016 •

edited

Loading