[drupal-devel] Rewriting use of forms in Drupal
Hi there! What follows is a proposal I sent to Dries before the security releases were made. Since it hinted at the possibility of flaws in our current way of handling forms I didn't want to make it available for public viewing at that time. There are probably still errors in some forms, but the most serious exploits should be fixed now. Although the proposal is geared towards node forms, it could be easily extended for other forms. Feedback would be appreciated. Instead of $output .= form_textfield(t('Title'), 'title', $edit->title, 60, 128, NULL, NULL, TRUE); we'd write $output['title'] = array('type' => 'textfield', 'title' => t('Title'), 'name' => 'title', 'value' => $edit->title, 'size' => 60, 'maxlenght' => 128, 'decription' => NULL, 'attributes' => NULL, 'required' => TRUE ); (We could add a 'group' element to indicate elements that should be grouped together, also a weight). Only in form() itself we'd generate the html. form() should be a themable function. What would that buy us? a) Easier value checking. In _validate we would then build the form a second time to check that all the fields still have the type and value they are supposed to have. foreach ($form_elements as $name => $value) { switch($value['type']) { case 'textfield': if (!is_string($edit['value'])) { form_set_error(...); } if (is_empty($edit['value']) && $value['required']) { form_set_error(...); } break; case 'textarea': .... } } (special checks such as "authored by" can be done here, possibly add another hook to get cleaner code) This would eliminate form problems such as the ones we are fixing in the bugfix release. Currently, a malicious user could try to spoof some of the fields (I am unsure with what kind of success). b) easier re-ordering of forms by modules and themes. Example: I'd like to add a description field to uploads uploaded through upload.module. I can do that through nodeapi(form post), but if I have three upload fields all my three description fields would appear after the three file selectors. If the existing form array would be passed around in "form post" I could add my fields in between. I think that Chris' ideas of nicer admin screens (with all their possible flaws) also require such a reorganization. Cheers, Gerhard
On 03/06/05, Gerhard Killesreiter <killesreiter@physik.uni-freiburg.de> wrote:
Instead of
$output .= form_textfield(t('Title'), 'title', $edit->title, 60, 128, NULL, NULL, TRUE);
we'd write
$output['title'] = array('type' => 'textfield', 'title' => t('Title'), [...] );
The biggest problem I've come across with this method is that it provides incredibly little flexibility in the form layout. You're almost completedly restricting yourself to a plain old linear form, with no side-by-side controls or anything else. For example, my recipe module has a table in it. Each row of that table has 3 input fields in it. I dread to think what the code looks like to generate that with an array of controls. -- David Carrington
On Fri, 3 Jun 2005, David Carrington wrote:
On 03/06/05, Gerhard Killesreiter <killesreiter@physik.uni-freiburg.de> wrote:
Instead of
$output .= form_textfield(t('Title'), 'title', $edit->title, 60, 128, NULL, NULL, TRUE);
we'd write
$output['title'] = array('type' => 'textfield', 'title' => t('Title'), [...] );
The biggest problem I've come across with this method is that it provides incredibly little flexibility in the form layout. You're almost completedly restricting yourself to a plain old linear form, with no side-by-side controls or anything else.
The intend of my proposal is to allow for this type of form layouts.
For example, my recipe module has a table in it. Each row of that table has 3 input fields in it. I dread to think what the code looks like to generate that with an array of controls.
I currently have no solution for this. As my proposal is now, you'd need to tell your theme what to do with the individual fields. An idea is to have form() (which would become theme_form()) call a "form layout" hook. Hmmmm. Cheers, Gerhard
On Friday 03 June 2005 07:09, Gerhard Killesreiter wrote:
For example, my recipe module has a table in it. Each row of that table has 3 input fields in it. I dread to think what the code looks like to generate that with an array of controls.
I currently have no solution for this. As my proposal is now, you'd need to tell your theme what to do with the individual fields. An idea is to have form() (which would become theme_form()) call a "form layout" hook. Hmmmm.
Take a page from the proverbial "Book of Java" -- delegate form layout into a layout management object. In Java, you pick a layout manager class for your form. That class defines the overall algorithm of how fields will be laid out, e.g., linear vertical, linear horizontal, grid of MxN cells, etc. (there are more complex layouts avaiable). Within each of these layout managers, one can assign a "weight" and an ordering number to each cell. In Java, the "weight" defines the proportion of space that is allocated to the cell, not its position relative to the others -- the order of cells is set by their index order. The *real* versatility of this comes from the fact that the layout managers can be nested. In other words, I can do a linear vertical array where the top cell is a single field, the second cell down is a horizontal array of three fields in a sub-layout, the third cell down is a 2x2 grid with 65%/35% left/right proportion and 50%/50% top/bottom proportion, and so on. The size (again, "weight" in Java parlance) and ordering factors are applied at each nested level of layout manager, and they can be nested to arbitrary depth. At each level of nesting, the layout manager in charge simply believes it is layout out one or more fields. Some fields, however, may actually be layout managers. This is, if you will, simply a hyper-extension of Drupal's existing form_group() concept. The size ("weight") and ordering parameters given to the layout managers with each contained field are advisory rather than mandatory. The layout manager makes a "best effort" to deliver what the programmer wants, based on constraints of the environment such as screen size. To bring this model into Drupal terms, what you would have is this: * Drupal core defines layout.module which establishes the API calls for how layout managers relate to one another, to the application, and to themes. * layout.module "knows" how to load one or more *.layout (for example) PHP files that each define a single layout manager type ("grid", "linear", etc.). This is akin to the architecture of flexinode.module or our theme engines. * Several basic core layouts are provided as part of Drupal itself, to provide for all the layout cases we have today in mainstream modules. Contributors can put additional .layout files in with their module directories, if they need something heavily customized. Or someone can create a really slick layout manager and contribute it standalone. * At runtime, the application programmer builds the lowest level layout objects and then nests them into higher level layouts, until the full form is defined (note: for most simple cases, there would be no nesting, just a single layout with all the fields inserted linearly). Each layout cell is filled with themeable HTML generated by today's existing form_*() functions or custom code, or themeable HTML generated by a sub-layout. * The layout modules take each of their cells' HTML contents and "wrap" them in the surrounding container tags, with appropriate CSS class and/or ID attributes, and generate the final HTML for the form. The class and ID names are standardized as part of the published API spec for layout modules. Layouts also get one uniquely-named class that they wrap (using "<div>", probably) around their entire output, and another class that they wrap around each of their cells' contents. This allows minimal CSS files for the use of layout modules themselves. * The full form HTML is passed to the theme engine as it is today. The theme defines how that form fits into the page and also the CSS for all the class and ID attributes. * Since the CSS (if any) from the layout is outside the CSS from the theme, the theme has the "final say" in how things are laid out, but if it is silent on a particular feature, the CSS from the layout manager comes through. This is admittedly a rough proposal, but the general philosophy has been tried- and-true in the Java world for close to a decade. I've done quite a bit of coding in Java and have found the system to be great for giving me control of how fields are placed relative to one another, without getting bogged down in the details of pixel counts and screen size. Scott -- ------------------------------------------------------------------------------- Scott Courtney Drupal user name: "syscrusher" http://drupal.org/user/9184 scott at 4th dot com Drupal projects: http://drupal.org/project/user/9184 Sandbox: http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/syscrusher
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03 Jun 2005, at 2:34 PM, Syscrusher wrote:
Take a page from the proverbial "Book of Java" -- delegate form layout into a layout management object. Err , I'd rather we look at how XUL / mozilla handles forms.
Emulating swing in html/css seems like the worst possible course of action. Does anyone here know struts? (which is closer to what we want to do, afaik). - -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCoFbugegMqdGlkasRAtIyAJ0dBbaAtDssbWHKMKux9KSiSKIuqQCfa1Sm LEHlIBT9dtWWVtroK98G5Iw= =/2C7 -----END PGP SIGNATURE-----
Take a page from the proverbial "Book of Java" -- delegate form layout into a layout management object. Err , I'd rather we look at how XUL / mozilla handles forms.
Emulating swing in html/css seems like the worst possible course of action. Does anyone here know struts? (which is closer to what we want to do, afaik). struts is a bit weird, probably a great technology though.
I think with the theme_* interface drupal already has a good approach. The biggest problem with forms in Drupal is that the "separation of concerns" is not clear. I like Gerhard's proposal because it would allow to decouple forming [sorry] the form structure from preparing the form presentation. The only slippery thing I can see is that it might make the form templates more complicated for designers. But they are not very useful at the moment. ergo: +1 Vlado
On Friday 03 June 2005 09:11, Adrian Rossouw wrote:
Emulating swing in html/css seems like the worst possible course of action.
What I'm proposing is more like a subset of AWT than like Swing. Swing is overkill for our needs, IMO. Scott -- ------------------------------------------------------------------------------- Scott Courtney Drupal user name: "syscrusher" http://drupal.org/user/9184 scott at 4th dot com Drupal projects: http://drupal.org/project/user/9184 Sandbox: http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/syscrusher
Robert Douglass wrote:
OT humor:
Actually, that doesn't seem off topic at all. It seems very applicable to our situation. :-) Let's be careful to not muddy the relationship between content generation and presentation layers. -- Chris Johnson
Emulating swing in html/css seems like the worst possible course of action.
What I'm proposing is more like a subset of AWT than like Swing. Swing is overkill for our needs, IMO. Is it html/css friendly?
for coding desk apps, I prefer the libglade approach - get an xml file and connect the callbacks. For web apps - the drupal model is very good. The suggested approach extends that/can extend to forms.
On Friday 03 June 2005 10:33, vlado wrote:
What I'm proposing is more like a subset of AWT than like Swing. Swing is overkill for our needs, IMO. Is it html/css friendly?
It can be, if correctly implemented. What you would probably want to do is to build the cell layout using tables, rather than CSS, but *only* to define the relationships among the cells. CSS emitted by the layout manager would be used to set the spacial attributes of the rows and cells. The theme would have its own CSS for things like text indentation, fonts, colors, shading, borders, and so forth that don't affect how the various fields are actually laid out relative to one another. In other words, the table only enforces that "there are three cells vertically, and the center one contains two horizontal cells in a 75/25 arrangement. The rest is up to CSS. Scott -- ------------------------------------------------------------------------------- Scott Courtney Drupal user name: "syscrusher" http://drupal.org/user/9184 scott at 4th dot com Drupal projects: http://drupal.org/project/user/9184 Sandbox: http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/syscrusher
On Fri, 3 Jun 2005, Gerhard Killesreiter wrote:
On Fri, 3 Jun 2005, David Carrington wrote:
The biggest problem I've come across with this method is that it provides incredibly little flexibility in the form layout. You're almost completedly restricting yourself to a plain old linear form, with no side-by-side controls or anything else.
The intend of my proposal is to allow for this type of form layouts.
For example, my recipe module has a table in it. Each row of that table has 3 input fields in it. I dread to think what the code looks like to generate that with an array of controls.
I currently have no solution for this. As my proposal is now, you'd need to tell your theme what to do with the individual fields. An idea is to have form() (which would become theme_form()) call a "form layout" hook. Hmmmm.
I've discussed a bit with Adrian. I think the current solution would be to add simple form items to a form group in _nodeapi('form pre') and then in your theme do the table layout for that group. The backdraw would be that all people would have to modify their themes to use your module. Themers to the front: Can we avoid this? Cheers, Gerhard
$output['title'] = array('type' => 'textfield', 'title' => t('Title'), 'name' => 'title', 'value' => $edit->title, 'size' => 60, 'maxlenght' => 128, 'decription' => NULL, 'attributes' => NULL, 'required' => TRUE );
I suspect that if someone would make a statistics of drupaldocs.org form_* would be on top. To be free from arbitrary parameter order would be a HUGE plus. Gerhard, if you need a hand in coding, you know where to find me.
Op 3-jun-2005, om 11:23 heeft Gerhard Killesreiter het volgende geschreven:
Instead of
$output .= form_textfield(t('Title'), 'title', $edit->title, 60, 128, NULL, NULL, TRUE);
we'd write
$output['title'] = array('type' => 'textfield', 'title' => t('Title'), 'name' => 'title', 'value' => $edit->title, 'size' => 60, 'maxlenght' => 128, 'decription' => NULL, 'attributes' => NULL, 'required' => TRUE );
I _really_ like this way of doing code. it's very easy to understand and makes more sense imo than the current way we do things.. Gerhard, If you need help with coding please e-mail me.. I'm very willing to help you.. Stefan.
On 03 Jun 2005, at 11:23, Gerhard Killesreiter wrote:
What follows is a proposal I sent to Dries before the security releases were made. Since it hinted at the possibility of flaws in our current way of handling forms I didn't want to make it available for public viewing at that time. There are probably still errors in some forms, but the most serious exploits should be fixed now. Although the proposal is geared towards node forms, it could be easily extended for other forms.
I think I'm missing the point. What _exactly_ do we gain? -- Dries Buytaert :: http://www.buytaert.net/
On Fri, 3 Jun 2005, Dries Buytaert wrote:
On 03 Jun 2005, at 11:23, Gerhard Killesreiter wrote:
What follows is a proposal I sent to Dries before the security releases were made. Since it hinted at the possibility of flaws in our current way of handling forms I didn't want to make it available for public viewing at that time. There are probably still errors in some forms, but the most serious exploits should be fixed now. Although the proposal is geared towards node forms, it could be easily extended for other forms.
I think I'm missing the point. What _exactly_ do we gain?
1) We can ensure that only the fields that are defined by our PHP code can be used. Currently, you can add html to any Drupal generated HTML form and the values will be processed and possibly be inserted into the database. We can also check if the fields still are of the type that they should. I am not sure that you can gain anything by exchaning a textarea input to a radioselect, but the possibility annoys me. 2) Themes could change the order and placement of fields. You could decide to generate your taxonomy tree somewhere else than between title and body. Maybe you can already do this through CSS, don't know. 3) People seem to like arrays more than strings. ;) Cheers, Gerhard
+1 on the direction. Seems like it would also make things like auto-generating and hiding titles easier. Robert
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 This brings it to a similar format as the menus. Perhaps we should build it so that you set up the element array, and then you have a form_element($elements['key']); function, which allows you to place it anywhere on the screen. So you would have a form('module_formname', $elements); function theme_form_module_formname($elements) { /* place the form elements how you want them using form_element ($elements['id'])*/ } You could also do it in a phptemplate format, so you could distribute customized forms. On 03 Jun 2005, at 1:49 PM, Robert Douglass wrote:
+1 on the direction. Seems like it would also make things like auto- generating and hiding titles easier.
Robert
- -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCoEocgegMqdGlkasRAmTzAJ9kcto0c8Dfpgf8+h/WDf2fVzBcRACg4YXB x76OBFtMr8yiUl5GPt1Euos= =qraM -----END PGP SIGNATURE-----
1) We can ensure that only the fields that are defined by our PHP code can be used.
Currently, you can add html to any Drupal generated HTML form and the values will be processed and possibly be inserted into the database. We can also check if the fields still are of the type that they should. I am not sure that you can gain anything by exchaning a textarea input to a radioselect, but the possibility annoys me.
With the exception of the node, comment (?) and user forms, no forms behave like that. What other forms behave like this?
2) Themes could change the order and placement of fields. You could decide to generate your taxonomy tree somewhere else than between title and body. Maybe you can already do this through CSS, don't know.
If that is the case, we need to approach it from a themer's perspective and figure out _how_ he want to shuffle form items around. Then we can worry about the implementation details.
3) People seem to like arrays more than strings. ;)
That's not an argument. -- Dries Buytaert :: http://www.buytaert.net/
On Fri, 3 Jun 2005, Dries Buytaert wrote:
1) We can ensure that only the fields that are defined by our PHP code can be used.
Currently, you can add html to any Drupal generated HTML form and the values will be processed and possibly be inserted into the database. We can also check if the fields still are of the type that they should. I am not sure that you can gain anything by exchaning a textarea input to a radioselect, but the possibility annoys me.
With the exception of the node, comment (?) and user forms, no forms behave like that.
Well, those forms are quite important. :)
What other forms behave like this?
Even if the answer (which I don't know) is "none" I'd still like to fix the other forms.
2) Themes could change the order and placement of fields. You could decide to generate your taxonomy tree somewhere else than between title and body. Maybe you can already do this through CSS, don't know.
If that is the case, we need to approach it from a themer's perspective and figure out _how_ he want to shuffle form items around. Then we can worry about the implementation details.
Since I am not a themer I've taken the only approach I am capable of. Chris had made some mockups a while ago. I guess we could ask ourselves if we can make them happen with my approach and then decide if it is the direction we want to go. My approach has the advantage that themers do not even need to agree on what the best order or layout of form elements would be as they could modify anything that Drupal sends them in the theme. Of course, it is desirable to have a default that is good and usually does not need modifications. We should however allow themers as much flexibility as we can. For example, the best place of some form element could depend on the writing direction of the language used. We would want to allow themers to accomodate this.
3) People seem to like arrays more than strings. ;)
That's not an argument.
Well, I think it is. As Karoly has pointed out it is quite difficult to remember the right order of the numerous arguments of the various form functions. I always have to look it up. Using an array would make this a non-issue. Making coding for Drupal even easier is certainly an argument. Cheers, Gerhard
3) People seem to like arrays more than strings. ;)
That's not an argument.
Well, I think it is. As Karoly has pointed out it is quite difficult to remember the right order of the numerous arguments of the various form functions. I always have to look it up. Using an array would make this a non-issue. Making coding for Drupal even easier is certainly an argument.
I agree with Gerhard.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03 Jun 2005, at 3:59 PM, Robert Douglass wrote:
Well, I think it is. As Karoly has pointed out it is quite difficult to remember the right order of the numerous arguments of the various form functions. I always have to look it up. Using an array would make this a non-issue. Making coding for Drupal even easier is certainly an argument. I agree with Gerhard. It's also more consistent with how menu's work (sans the recursive nesting algorithm, we hope), and is the first step towards the CCK form editor.
I have a suspicion it will lead to cleaner code, and better seperation of content / logic. Perhaps we can even have a 'default'=> setting, that allows us to get rid of all the $var = ($_GET['variable']) ? $_GET['variable'] : variable_get('variable', 'default'); lines - -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCoGpBgegMqdGlkasRAiiHAJ49Fz2D51rIPQiSEiLeMldhFmBgvQCeJ4tk XnTafAuYucXrjqTsGP0KIE8= =n2y6 -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Here's what I am envisioning : /* This is the entire form definition */ function somemodule_formname($obj) { $elements['title'] = array('type'=>'textfield', 'title'=> t ('Title'), $value= $_POST['edit']['title'], 'default' => $obj->title, 'weight' => 0); $elements['group1'] = array('weight' => 1, 'title' => t('some title here')); $elements['group1']['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), $value= $_POST['edit']['group1'] ['element1'], 'default' => $obj->element1, 'weight' => 0); $elements['group1']['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), $value= $_POST['edit']['group1'] ['element1'], 'default' => $obj->element2, 'weight' => 1); return form('somemodule_form', $elements); } /* this goes in common.inc */ function form($name, $elements) { if ($form = theme('somemodule_formname', $elements)) { return $form; } else { return form_default_renderer($elements); } } function form_default_renderer($elements) { /* Sort elements via weight */ foreach (array_keys($elements) as $key) { $form .= form_element($key); } return $form; } function form_element($element) { if (/*test for group here*/) { $form = form_group(form_default_renderer($element), $element ['title']); $form = form_group($group, $element['title']); } else { $form = ${'form_' . $element->type}($element); // Redirects to the type specific element could also be a switch inside form_element } return $form; } What could then happen, is the module could define it's own form layout, by doing : function theme_modulename_formname($elements) { $form .= form_element($element['title']); $form .= form_element($element['group1']); /* renders entire group */ return $form; } This could of course be overridden. Say you didn't want to display the title , you could do a function mytheme_modulename_formname($elements) { $elements['title']['type'] => 'hidden'; return form_default_renderer($elements); } The biggest problem with this is the use of 'title' and 'weight' and 'type' and the like, since you could have an element which is called 'type' , so what I would do is use constants for the keys .. ie TYPE = 'group' , TITLE => 'title goes here' etc. - -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCoHKqgegMqdGlkasRAhCAAJ9VA18IQxZTxyKFehy4hY5eC5jtmQCfXbYU rf42MWxR7sXbddx72gNmlSE= =Snue -----END PGP SIGNATURE-----
On Fri, 3 Jun 2005, Adrian Rossouw wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Here's what I am envisioning :
/* This is the entire form definition */ function somemodule_formname($obj) { $elements['title'] = array('type'=>'textfield', 'title'=> t ('Title'), $value= $_POST['edit']['title'], 'default' => $obj->title, 'weight' => 0); $elements['group1'] = array('weight' => 1, 'title' => t('some title here'));
I'd rewrite the last part to $elements['group1'] = array('type' => 'group', 'weight' => 1, 'title' => t('some title here'));
$elements['group1']['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), $value= $_POST['edit']['group1'] ['element1'], 'default' => $obj->element1, 'weight' => 0); $elements['group1']['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), $value= $_POST['edit']['group1'] ['element1'], 'default' => $obj->element2, 'weight' => 1);
And then for elements: $elements['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), 'value' => $_POST['edit']['element1'], 'default' => $obj->element1, 'weight' => 1, 'group' => 'group1'); This avoids nested arrays which is a good thing.
return form('somemodule_form', $elements); }
/* this goes in common.inc */
function form($name, $elements) { if ($form = theme('somemodule_formname', $elements)) { return $form; } else { return form_default_renderer($elements); } }
Looks good.
function form_default_renderer($elements) { /* Sort elements via weight */
foreach (array_keys($elements) as $key) { $form .= form_element($key); } return $form; }
function form_element($element) { if (/*test for group here*/) { $form = form_group(form_default_renderer($element), $element ['title']); $form = form_group($group, $element['title']); } else { $form = ${'form_' . $element->type}($element); // Redirects to the type specific element could also be a switch inside form_element } return $form; }
My change above makes this code simpler too.
What could then happen, is the module could define it's own form layout, by doing :
function theme_modulename_formname($elements) { $form .= form_element($element['title']); $form .= form_element($element['group1']); /* renders entire group */ return $form; }
This would take care of David's problem.
This could of course be overridden. Say you didn't want to display the title , you could do a
function mytheme_modulename_formname($elements) { $elements['title']['type'] => 'hidden'; return form_default_renderer($elements); }
I am not sure we should allow modules to change form types.
The biggest problem with this is the use of 'title' and 'weight' and 'type' and the like, since you could have an element which is called 'type' , so what I would do is use constants for the keys .. ie TYPE = 'group' , TITLE => 'title goes here' etc.
I think I solved this problem, too. Cheers, Gerhard
I'd rewrite the last part to
$elements['group1'] = array('type' => 'group', 'weight' => 1, 'title' => t('some title here'));
And then for elements: $elements['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), 'value' => $_POST['edit']['element1'], 'default' => $obj->element1, 'weight' => 1, 'group' => 'group1');
How would you handle nested groups?
On Fri, 3 Jun 2005, vlado wrote:
I'd rewrite the last part to
$elements['group1'] = array('type' => 'group', 'weight' => 1, 'title' => t('some title here'));
And then for elements: $elements['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), 'value' => $_POST['edit']['element1'], 'default' => $obj->element1, 'weight' => 1, 'group' => 'group1');
How would you handle nested groups?
Do we have nested groups? Wasn't aware of this. I guess a group could be member of another group: $elements['group1'] = array('type' => 'group', 'group' => 'group2', 'weight' => 1, 'title' => t('some title here')); Cheers, Gerhard
On Fri, 2005-06-03 at 17:32 +0200, Gerhard Killesreiter wrote:
On Fri, 3 Jun 2005, vlado wrote:
I'd rewrite the last part to
$elements['group1'] = array('type' => 'group', 'weight' => 1, 'title' => t('some title here'));
And then for elements: $elements['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), 'value' => $_POST['edit']['element1'], 'default' => $obj->element1, 'weight' => 1, 'group' => 'group1');
How would you handle nested groups?
Do we have nested groups? Wasn't aware of this.
It can solve a lot of problems with logically extending forms, from nodeapi. for example event->recursion rule ->location->address ....
I guess a group could be member of another group:
$elements['group1'] = array('type' => 'group', 'group' => 'group2', 'weight' => 1, 'title' => t('some title here')); That can work, but not sure if it will make the code simpler
Nested arrays in this case are natural to describe the form structure, so IMO they will require less logic to do the form presentation later.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03 Jun 2005, at 5:24 PM, Gerhard Killesreiter wrote:
On Fri, 3 Jun 2005, Adrian Rossouw wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Here's what I am envisioning :
/* This is the entire form definition */ function somemodule_formname($obj) { $elements['title'] = array('type'=>'textfield', 'title'=> t ('Title'), $value= $_POST['edit']['title'], 'default' => $obj->title, 'weight' => 0); $elements['group1'] = array('weight' => 1, 'title' => t('some title here'));
I'd rewrite the last part to
$elements['group1'] = array('type' => 'group', 'weight' => 1, 'title' => t('some title here'));
$elements['group1']['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), $value= $_POST['edit']['group1'] ['element1'], 'default' => $obj->element1, 'weight' => 0); $elements['group1']['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), $value= $_POST['edit']['group1'] ['element1'], 'default' => $obj->element2, 'weight' => 1);
And then for elements: $elements['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), 'value' => $_POST['edit']['element1'], 'default' => $obj->element1, 'weight' => 1, 'group' => 'group1');
This avoids nested arrays which is a good thing.
I thought of the same, but what I like about my approach, is that there is less sorting required, and because there is only one form_ function that the themer needs to know, it will always work. He can print the entire group by just going form_element($element ['group1']), and just an element out of the group using form_element($element ['group1']['element1']); I suppose it could work using form_element($elements,'group1') or form_element($elements, 'element1'), but it would require more sorting inside the code.. ie: stepping through each element to find all the elements that need to be grouped together. It's 6 of one, and half a dozen of the other.. and I'm not particularly stuck on either approach.
/* this goes in common.inc */
function form($name, $elements) { if ($form = theme('somemodule_formname', $elements)) { return $form; } else { return form_default_renderer($elements); } }
Looks good.
function form_element($element) { if (/*test for group here*/) { $form = form_group(form_default_renderer($element), $element ['title']); $form = form_group($group, $element['title']); } else { $form = ${'form_' . $element->type}($element); // Redirects to the type specific element could also be a switch inside form_element } return $form; }
My change above makes this code simpler too. As I explained, I think it might make the code more complex.
This could of course be overridden. Say you didn't want to display the title , you could do a
function mytheme_modulename_formname($elements) { $elements['title']['type'] => 'hidden'; return form_default_renderer($elements); }
I am not sure we should allow modules to change form types. This could be really useful.
Imagine if you install an extra module which changes a field to autocomplete, and adds a callback. Or perhaps a module that adds a javascript date picker that replaces the form_date with a form_javascript_date. As long as the validation doesn't break, I don't foresee this being a problem. The pathauto module might decide to remove the path field completely in a node form. Another point is, the nodeapi in this case turns into a single call 'form' which allows you to add elements to the form array, as well as changing items as described.
I think I solved this problem, too.
What if we have a TYPE='container' , as per Ber's recent patch. We might also need deeper nesting (not that I would like to see that in core) - -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCoHl9gegMqdGlkasRAldCAKDUDFJLBqqysTzzdo2UwsXbeQsXBgCfZKhz X1L7g+BKa/mWF6Vctb+AXuo= =ZECx -----END PGP SIGNATURE-----
On Fri, 3 Jun 2005, Adrian Rossouw wrote:
On 03 Jun 2005, at 5:24 PM, Gerhard Killesreiter wrote:
On Fri, 3 Jun 2005, Adrian Rossouw wrote:
$elements['group1']['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), $value= $_POST['edit']['group1'] ['element1'], 'default' => $obj->element1, 'weight' => 0); $elements['group1']['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), $value= $_POST['edit']['group1'] ['element1'], 'default' => $obj->element2, 'weight' => 1);
And then for elements: $elements['element1'] = array('type'=>'textfield', 'title'=> t('element 1'), 'value' => $_POST['edit']['element1'], 'default' => $obj->element1, 'weight' => 1, 'group' => 'group1');
This avoids nested arrays which is a good thing.
I thought of the same, but what I like about my approach, is that there is less sorting required,
That is true. But I think the code looks cleaner the other way around. It is also not a performance concern as forms would only rendered on form pages.
and because there is only one form_ function that the themer needs to know, it will always work.
He can print the entire group by just going form_element($element ['group1']), and just an element out of the group using form_element($element ['group1']['element1']);
I suppose it could work using form_element($elements,'group1') or form_element($elements, 'element1'),
I think having a simple interface to form elements would work either way.
but it would require more sorting inside the code.. ie: stepping through each element to find all the elements that need to be grouped together.
It's 6 of one, and half a dozen of the other.. and I'm not particularly stuck on either approach.
Neither am I.
$form = ${'form_' . $element->type}($element); // Redirects to the type specific element could also be a switch inside form_element } return $form; }
My change above makes this code simpler too.
As I explained, I think it might make the code more complex.
The code in common.inc (or wherever it will be, I'll probably make a form.inc) will be more complex, I agree. But the actual form building code in the modules will be easier to read if we do not have nested arrays.
Say you didn't want to display the title , you could do a
function mytheme_modulename_formname($elements) { $elements['title']['type'] => 'hidden'; return form_default_renderer($elements); }
I am not sure we should allow modules to change form types. This could be really useful.
Imagine if you install an extra module which changes a field to autocomplete, and adds a callback.
Shouldn't we then rather patch the original module?
Or perhaps a module that adds a javascript date picker that replaces the form_date with a form_javascript_date.
If we can have a non-JS friendly date picker it should be in core, if we cannot, we shouldn't support it.
As long as the validation doesn't break, I don't foresee this being a problem.
The validation could break, that is why I am not convinced changing field types is a good idea.
The pathauto module might decide to remove the path field completely in a node form.
Hmm. You can get that with permissions. I guess if we can figure out how to validate with changing form types, we could support it since it is according to my "Drupal should be as flexible as possible" mantra.
Another point is, the nodeapi in this case turns into a single call 'form' which allows you to add elements to the form array, as well as changing items as described.
I think I solved this problem, too.
What if we have a TYPE='container' , as per Ber's recent patch.
That would be the same as a form group, no?
We might also need deeper nesting (not that I would like to see that in core)
If we can make usefull use of it, why not? Cheers, Gerhard
On 03 Jun 2005, at 17:09, Adrian Rossouw wrote:
Here's what I am envisioning :
Allright, I'm convinced. :) -- Dries Buytaert :: http://www.buytaert.net/
On Fri, Jun 03, 2005 at 06:33:32PM +0200, Dries Buytaert wrote:
Here's what I am envisioning : Allright, I'm convinced. :)
from a user prospective I'm not convinced. I've used this paradigm in java and using few php html (PHTML.php for example) libraries with postnuke (and I'm glad I don't have to use them anymore). I think this is an over complication for drupal. Writing modules (in my experience) does never require to have an uber-complex layout. Most of the time it's just a simple form (no nesting). Anything more complex can be achieved via css or a very simple table (I'll be happy to see a module that actually require a layout with severl level of nested layout). The current system is simple and well tailored for what drupal does. In java or xul is much more common to write complex layout because of the nature of the applications. Moreover I'm a lazy programmer and this proposal would require more typing. I personally love the way it is. :) p -- ++ Blog: http://blog.rsise.anu.edu.au/?q=pietro ++ ++ "All great truths begin as blasphemies." -George Bernard Shaw ++ Please avoid sending me Word or PowerPoint attachments. See http://www.fsf.org/philosophy/no-word-attachments.html
of the applications. Moreover I'm a lazy programmer and this proposal would require more typing. I personally love the way it is.
Well, it'll definitely need a bit more typing with spelling out the names of different elements -- but it'll result in a much more easily understandable key. Quick, tell me, in form_textarea is the order cols, rows or rows, cols?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 04 Jun 2005, at 4:46 PM, Karoly Negyesi wrote:
of the applications. Moreover I'm a lazy programmer and this proposal would require more typing. I personally love the way it is.
Well, it'll definitely need a bit more typing with spelling out the names of different elements -- but it'll result in a much more easily understandable key. Quick, tell me, in form_textarea is the order cols, rows or rows, cols?
Or which parameter number the $description variable is in form_textfield, versus in form_radio ? - -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCocMPgegMqdGlkasRAqjTAJ9Gsi+5JoYIfpArU0YEzo7YPJ2OkACgyF2Z +xRvjH5nA2PPruxnLJbX0EQ= =MPHX -----END PGP SIGNATURE-----
a) Easier value checking.
In _validate we would then build the form a second time to check that all the fields still have the type and value they are supposed to have.
foreach ($form_elements as $name => $value) { switch($value['type']) { case 'textfield': if (!is_string($edit['value'])) { form_set_error(...); } if (is_empty($edit['value']) && $value['required']) { form_set_error(...); } break; case 'textarea': .... } }
Who does the validation? Core, or the module? How would that work for a form outside of the node forms? ... The direction sounds good so far.
On Fri, 3 Jun 2005, Moshe Weitzman wrote:
a) Easier value checking.
In _validate we would then build the form a second time to check that all the fields still have the type and value they are supposed to have.
foreach ($form_elements as $name => $value) { switch($value['type']) { case 'textfield': if (!is_string($edit['value'])) {
That should have been $edit[$value['name']].
form_set_error(...); } if (is_empty($edit['value']) && $value['required']) { form_set_error(...); } break; case 'textarea': .... } }
Who does the validation? Core, or the module?
This kind of basic validation should be in core.
How would that work for a form outside of the node forms? ...
Just the same.
The direction sounds good so far.
Thanks. Cheers, Gerhard
Gerhard Killesreiter wrote:
Hi there!
What follows is a proposal I sent to Dries before the security releases were made. Since it hinted at the possibility of flaws in our current way of handling forms I didn't want to make it available for public viewing at that time. There are probably still errors in some forms, but the most serious exploits should be fixed now. Although the proposal is geared towards node forms, it could be easily extended for other forms.
Feedback would be appreciated.
Actually, I discovered a something related issue today... it is in fact impossible to do this in Drupal: $node = node_load(...); node_validate($node); node_save($node); The problem is that in node_validate() we assume that we only get fields which were output in forms. Some fields, like the teaser, are by default not output (only if you have excerpt.module). node_validate() checks if there is a teaser provided already (assuming any teaser comes from an external module), and if so, doesn't generate a new one. Similar problems can exist, for example when a contributed module does not have a single body field. After submitting the node_form(), there would be no $node->body, but this field is present after you do a node_load(). In node_validate we would need to unset any field which is not part of the standard form. Your technique seems to open the door for that. Steven Wittens
Only in form() itself we'd generate the html. form() should be a themable function.
A great step forward--better control, more flexibility. Anything that moves us away from hard-coded HTML generation and toward separation of content and presentation is a good thing. The "weight" parameter suggestion sounds good. I've often been frustrated at not having control over the order of form elements (e.g., not being able to insert new elements at a specified place).
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03 Jun 2005, at 6:17 PM, Nedjo Rogers wrote:
Only in form() itself we'd generate the html. form() should be a themable function.
A great step forward--better control, more flexibility. Anything that moves us away from hard-coded HTML generation and toward separation of content and presentation is a good thing.
The "weight" parameter suggestion sounds good. I've often been frustrated at not having control over the order of form elements (e.g., not being able to insert new elements at a specified place).
Also, it opens up the ability to pass the elements of nodes to the theme_node as an array, meaning we can use this as a display mechanism too. - -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCoIHPgegMqdGlkasRApz4AJ4hfFWPV/fNYk5Kl9honPfCunOekACfZyPc lqlLIPWT0XXWaoX2aQTre70= =yvSk -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03 Jun 2005, at 6:17 PM, Nedjo Rogers wrote:
Only in form() itself we'd generate the html. form() should be a themable function.
A great step forward--better control, more flexibility. Anything that moves us away from hard-coded HTML generation and toward separation of content and presentation is a good thing.
The "weight" parameter suggestion sounds good. I've often been frustrated at not having control over the order of form elements (e.g., not being able to insert new elements at a specified place).
Furthermore, we should look at the admin section. We have tables containing inputs. Should we break the new method of seperating things and just use form_element(array()) , and how do we handle the <form> tag ? What I think should happen is we should create all the form elements, and then hand it over to a themeing function that generates the table, which leads me to think we might need to have element types for labels and images too (thinking of the theme selection screen). Perhaps a generic 'markup', or 'html' will suffice. Also, what I have planned would mean that each form has a unique name... which leaves us with the interesting thought that we could have a hook_actions() which allows us to set a callback for the form. IE: function module_actions() { $actions = array( 'module_form' => array('callback' => 'module_formname', 'access' => user_access('add some thing')) ); return $actions; } Then your form definition is as follows : function module_formname($action, $op, $edit = array()) { switch ($op) { case 'elements' : /* Define elements array here */ break; case 'validate' : /* Validate form here. */ return $valid; break; case 'execute' : /*sql or whatever goes here */ break; } } What does this mean to us ? It means that you can place any form anywhere simply using form('form name here'), it could even be possible to have multiple forms in the same <form> tag using form(array('form1', 'form2')); It would be possible too execute any action, whatsoever, from wherever (with the permission being checked on the action.) Furthermore, we could have a generic formapi to handle parts of nodeapi. This would mean you can add and remove fields from any form, including nodes, comments, admin screens (this could be great for people who want to generate their own admin pages). Done right, it could essentially be similar to the theme() functionality, but for all forms/actions. - -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCoK/ggegMqdGlkasRAmBfAJ4p3fcCM8V04IRPoG4lDw5X5GK3VQCcCFYW HTcisJmdRtd/hdOanFkgyic= =Yl8W -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03 Jun 2005, at 9:30 PM, Adrian Rossouw wrote:
Also, what I have planned would mean that each form has a unique name... which leaves us with the interesting thought that we could have a hook_actions() which allows us to set a callback for the form.
and that all sounds kind of what we need for 'http://drupal.org/node/ 24249' - -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCoLLtgegMqdGlkasRAvBLAJ9KHP4ArQIHkBa0iTMFpweK6G5AcACeJelt J+cEJrEP4i7u3oZQM54rjE8= =K3mG -----END PGP SIGNATURE-----
participants (14)
-
Adrian Rossouw -
Chris Johnson -
David Carrington -
Dries Buytaert -
Gerhard Killesreiter -
Karoly Negyesi -
Moshe Weitzman -
Nedjo Rogers -
Pietro Abate -
Robert Douglass -
Stefan Nagtegaal -
Steven Wittens -
Syscrusher -
vlado