Issue status update for http://drupal.org/node/29465 Post a follow up: http://drupal.org/project/comments/add/29465 Project: Drupal Version: cvs Component: base system Category: tasks Priority: critical Assigned to: adrian Reported by: adrian Updated by: jvandyk Status: patch (code needs work) Attachment: http://drupal.org/files/issues/forms_2.patch (47.72 KB) Here's an updated patch against HEAD that is mostly working. Karoly provided the updated node.module. For me, there seems to be an issue in form.inc in the recursive _form_builder() function where it is recursing even when $element is not an array but is a scalar. jvandyk Previous comments: ------------------------------------------------------------------------ Tue, 23 Aug 2005 11:34:00 +0000 : adrian Attachment: http://drupal.org/files/issues/form.inc (20.53 KB) This is the first check in of the new forms api code. The system has been designed to co-exist with the current forms api, and is contained in a new include file (includes/form.inc). Forms are now defined in their component arrays, similar to how menu items are defined. example : <?php $form['body'] = array(type => 'textarea', default_value => $node->body, cols => 60, rows => 60); ?> Elements can also be nested, and the $edit follows this definition. For instance : <?php $form['author'] = array(type => 'fieldset', title => t('Authoring information'), collapsible => TRUE, collapsed => TRUE, weight => -1); $form['author']['name'] = array(type => 'textfield', title => t('Authored by'), maxlength => 60, autocomplete_path => 'user/autocomplete', default_value => $node->name, weight => -1); ?> All the properties used are defined as constants, and are documented for each of the elements, and individually. ------------------------------------------------------------------------ Tue, 23 Aug 2005 11:46:19 +0000 : adrian A patch for node.module, blog.module and taxonomy.module that changes them to use the new form format. This patch is very far from complete, but I wanted to get the code out so that i'm not working alone anymore. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:08:01 +0000 : adrian Attachment: http://drupal.org/files/issues/forms.patch (9.98 KB) The actual patch =) I forgot to mention, this adds a new hook .. namely hook_elements, which allows us to define the defaults for the elements (ie : cols and rows for textareas) meaning they don't have to be defined for each of the elements. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:09:11 +0000 : chx A few notes from my conversation with adrian. valid => array('integer', 'uid') for this to work you need function valid_integer($element) and valid_uid($element). $extra for form_select is legacy and really needed. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:39:13 +0000 : fago i really like this approach. further i'd like to see the possibility to define an additional class to a form element, which is currently not working. so we 'd have to bring _form_get_class() and drupal_attributes() together. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:56:31 +0000 : adrian that works already. <?php $form[attributes]['class'] = 'someclass'; ?> Although I am considering just adding a class property ... ie: <?php $form[class][] = 'someclass'; ?> The fact that this is done via arrays, it means that the developer can add classes as he or she sees fit. ------------------------------------------------------------------------ Tue, 23 Aug 2005 13:29:10 +0000 : fago really? i don't think so. e.g. $checkbox = '<input type="checkbox" class="'. _form_get_class('form-checkbox', $element[required], _form_get_error($element[name])) .'" name="'. $element[name] .'" id="'. $element[id].'" value="'. $element[return_value] .'"'. ($element[value] ? ' checked="checked"' : '') . drupal_attributes($element[attributes]) .' />' so we will end up with two class attributes, which won't work and isn't standard compliance. your css property idea would be ideal imho. ------------------------------------------------------------------------ Tue, 23 Aug 2005 13:40:12 +0000 : nevets Minor point on #5 and #6, when accessing an associated array like $form[class][] = 'someclass'; if the key is a string it should be enclosed in quotes, i.e. $form['class'][] = 'someclass'; (This is from the PHP documentation.) ------------------------------------------------------------------------ Tue, 23 Aug 2005 14:35:29 +0000 : moshe weitzman Does this API affect form validation also? Thats the vague impression I had in my head, but I don't see any validation changed in node or taxonomy modules. perhaps that part is coming next. There are reasons to love this patch. But one thing I don't like is the movement toward arrays and away from functions. Modern editors and IDE's offer function tips and function completion. These are huge time and brain savers. They are great for newbies and for experts. It is so helpful to just type 'form_sel', press tab, and have form_select('title', 'name', 'value', 'options') printed for you, with all the arguments. When you define forms in an array instead of functions, as proposed in this patch, you lose a lot of developer productivity and friendliness for newbies. Developers are also more prone to mistakes this way since the editor can't guide them along. This is the sort of advantage that means nothing to the many people who just use a plain editor, and means everything to IDE users. Maybe someone can think of a way to keep the flexibility without losing IDE productivity. ------------------------------------------------------------------------ Wed, 24 Aug 2005 08:45:53 +0000 : adrian The api has a drupal_validate_form() function, which does the following validation : It steps through each of the elements, and executes any of the valid properties. An example would be valid => 'username'. It then calls valid_username($element), which can check for errors. It then calls $form_id_validate() , which can check for errors between form elements. It then (optionally) calls $callback_validate(), which allows you to have unique form id's , similar to how the example does the node form. You could create a function $type_node_form_validate(), to validate only that form, or a theme_$type_node_form() to theme that form differently. An example of where this would be used is for CCK, where it will have a single callback for all nodes created by it. Errors are flagged using form_error($element). It's different from form_set_error, in that it also sets the error property of the element, which I think is more practical than using the globals. Regarding the IDE discussion, I am on the fence about that, but definitely leaning towards preferring the arrays over the function calls. I think that the menu system has proven itself, and that it's better to be consistent. The plan for 4.7, is to leave the current form api in , so that all contrib modules don't need to be ported, but to switch core over to the new form system anyway, so for the time being .. the old functions will still exist. All this would be a non-issue if php supported named parameters, which is essentially what we are reproducing using arrays. ------------------------------------------------------------------------ Wed, 24 Aug 2005 12:36:08 +0000 : Thomas Ilsche I agree with moshe, and I think for day to day use the current forms api does a good job - however on complicated constructions i consider this to be really useful. The problem i see is to keep the whole forms api consistent and easy to learn, any ideas? I'd be against deprecating the current functions. About the keyword definitions. I think it should be consistent with for hook_menu and all its "named parameter" friends, and to at least not confuse it more define the keywords without the leading underscore. ------------------------------------------------------------------------ Wed, 24 Aug 2005 13:20:13 +0000 : adrian I'd prefer to make the menu system properties be consistent with the form system. actually. I know the conventional logic is that all constants being uppercase, and the first versions of the form code did stick to that, but the end result of the lowercase constants was far more readable code (the underscores however are a necessity to allow for nesting.) What i was thinking was, that we could use the conventional form api as constructors for the form array. ie: <?php $group .= form_textfield(t('File system path'), 'file_directory_path', $directory_path, 60, 255, t('desc here'), null, etc) ?> turns into <?php $form['files']['file_directory_path'] = form_textfield(t('File system path'), $directory_path, 60, 255, t('desc here'), null, etc); $form['files']['file_directory_path'][valid] = 'directory'; // any other properties that aren't in constructors. ?> instead of <?php $form['files']['file_create_path'] = array( type => 'textfield', title => t('File system path'), default_value => $directory_path, maxlength => 255, valid => 'directory', description => t('desc here') ); ?> and form_textfield turns into <?php function form_textfield($title, $value, $size, $maxlength, $description = NULL, $attributes = NULL, $required = FALSE) { return array( title => $title, size => $size, maxlength => $maxlength, description = $description, attributes => $attributes, required => $required); } ?> Benefits : 1) easier to port 2) the ide thingy Drawbacks : 1) more than one way to do something. 2) all forms will need to be upgraded, since the core form api will change. ie: breaks all of contrib. 3) Constructors could become unwieldy trying to tend to most of the parameters that can be set (weight, valid, validation_arguments, etc) ------------------------------------------------------------------------ Wed, 24 Aug 2005 13:47:17 +0000 : Dries There should only be one way to build forms. Simplicity and uniformity is king. For now, I leave it up to Adrian to decide what this "one way" is going to look like. (I like his initial approach. The code is shorter which saves time too.) ------------------------------------------------------------------------ Wed, 24 Aug 2005 14:24:48 +0000 : chx I second Dries. The old form API be gone. The IDE is going to be a problem, yes. A possible approach: the default array have all keys possible set to NULL or so. ------------------------------------------------------------------------ Wed, 24 Aug 2005 14:28:50 +0000 : Bèr Kessels I prefer the One Way too. having more ways to do something normally results in two half-witted ways, instead of one way that works As Best As Possible. I like the arrays approach. I love it, in fact. I have a feeling that the more code in drupal adopts the Array Way [tm], the more power AND uniformity we get. Just look at the success of array based menus: powerfull, yet simple to develop with. But, I have a few hesitations: one is the lowercase CONSTANTS. I know, this is good for readability, so I lean towards the side of: then just use lowercase constants. But still, something does not feel right about it. I think this needs some more though, or comments of others. Another thing I dislike is the way we use parameters to construct, IMO, completely different widgets. We should try to not think in terms of HTML, but in terms of usage and display. In HTML a collapsible form is similar -or nearly- to a noncollapsible. Same goes for a multi-select and none-multiselect select. But where I really think we should have different APIs is for autofills. They are IMHO not textfields, but a complete separate widget. Thus they should get a separate API. And last about the IDEs: allthough I do understand the problem, i beleive it is a very bad habit to let your code/application/product be limited, because of the tools you use. If your tools cannot handle libraries/snippets/etc beyond some function calls, IMO you should get anoter IDE :). But surely we should not let us be held back by these limitations in these IDEs. ------------------------------------------------------------------------ Wed, 24 Aug 2005 15:31:14 +0000 : moshe weitzman Attachment: http://drupal.org/files/issues/form_defaults.patch (2.39 KB) Um, snippits and macros are not a substitute for function complete. Snippits and macros are static entities which you manually create in the IDE and are then available as needed. If syntax changes for an given snippit, you have to manually change it. Thats just annoying enough to make you not use these feature at all. By contrast, function autocomplete just works the second you open a file. If you start working on a Contrib module you never seen before, the IDE introspects and is immediately ready for action. So all that nice PHPDoc that Adrian has written would be nicely used. Not so with snippits or macros. If you guys want to see what the fuss about IDE is, download the free trial of Zend Studio (http://www.zend.com/store/products/zend-studio/) or Komodo (http://www.activestate.com/Products/Komodo/ - note the small link for the OSX alpha. I've tried the alpha and it works). IDEs take a little while to get configured and get comfortable ... Create a project and start typing some common functions. See how the params show up and all our PHPDoc is there for your use. You can also quickly navigate your project via function names, and skip to the spot where a given function is defined. It is a tremendous time saver, and bug preventer. The IDEs above also offer a debugger which lets you step through your code and set breakpoints. Want to know the current value of a variable - just hover over it in your debugger. Once you've gotten the hang of this, you will never debug using print statements again. I see nothing wrong with designing Drupal so that it uses those PHP language features that are friendly to IDEs. Namely, functions, classes, constants, etc. Arrays are very flexible, but that flexibility comes at a price. ------------------------------------------------------------------------ Wed, 24 Aug 2005 16:34:41 +0000 : chx Attachment: http://drupal.org/files/issues/forms_chx.patch (15.59 KB) Like we have core.php for hooks we can have form.php for form API and let that help your hand with any IDE. Here is an updated version of Adrian's patch. User login block reworked. I move on to other parts of user.module. I also created a theme_password() function, so I will post form.inc as well. ------------------------------------------------------------------------ Wed, 24 Aug 2005 16:49:05 +0000 : chx Attachment: http://drupal.org/files/issues/form_0.inc (21.01 KB) ------------------------------------------------------------------------ Wed, 24 Aug 2005 22:23:06 +0000 : gordon +1 I like this patch a lot. but 2 things. * The testarea hook is not supported. This needs to be implemented. * the existing form_*() need to be changed to use the theme_*() so that there is only one place that form items can be created. I think this will be great for theme developers and it will be easier to build forms for module developers. Great job. ------------------------------------------------------------------------ Fri, 26 Aug 2005 09:27:08 +0000 : adrian Attachment: http://drupal.org/files/issues/form_1.inc (24.73 KB) Here is an updated version of the patch. 1) admin / settings has been rewritten 2) Added form_radios and form_checkboxes and form_select and a few others 3) Very strict validation now .. the drupal_get_form function automatically validates any input .. no way to skip that. If it doesn't produce errors, it calls $form_id_execute(), or the optional $callback_execute(). 4) Started the first part of the implementation of the multiple keyword, which is when an element can have multiple values. An example of this would be the 'files' element from upload.module , where more files get added, and also the primary / secondary links configuration of phptemplate. Still needs to be done : The clean_url validation, and any other module with a _settings hook. I haven't integrated chx' user module work at this point. ------------------------------------------------------------------------ Fri, 26 Aug 2005 09:48:14 +0000 : adrian Attachment: http://drupal.org/files/issues/forms_0.patch (37.68 KB) Here's the patch. I'm also still busy with the filter format, which has unique requirements and needs to be a filter_format element type (gets rid of the in-line html) ------------------------------------------------------------------------ Sun, 28 Aug 2005 02:52:59 +0000 : yogadex One thing I like about the current API is that you can inject arbitrary HTML in the middle of a form if you see fit. It's not obvious to me how to do that with the new API. Is there a way? Could I, for example, arrange my form fields in a table with two or three columns? ------------------------------------------------------------------------ Sun, 28 Aug 2005 07:26:13 +0000 : naudefj I would also like to be able to display forms in HTML tables. Here's a great article explaining how forms can be styled using tables - http://www.cs.tut.fi/~jkorpela/forms/tables.html ------------------------------------------------------------------------ Sun, 28 Aug 2005 08:25:06 +0000 : adrian EVERY form will now have a theme function, as every form now has it's own (unique) form-id. You can create a theme_my_form_id() function if you are a developer, and require the form to be differently layed out (like for instance, adding all the element outputs to a table). You can create a mytheme_my_form_id() if you are a theme developer, that can override the form layout. (as is shown by the current node_form) [? function theme_node_form($form) { $output .= ''; $output .= ''; $output .= ''; $output .= form_render($form['author']); $output .= ''; $output .= ''; $output .= form_render($form['options']); $output .= ''; $output .= ''; $output .= ''; $output .= form_render($form_render); $output .= ''; $output .= ''; return $output; } ?] You can create a phptemplate stub to load it from a template. <?php function phptemplate_node_form($element) { return _phptemplate_default('node_form', $element); } ?> and the current node_form template being : [? <?php print form_render($form['author']) ?> <?php print form_render($form['options']) ?> <?php print form_render($form) ?> ?] NOTE: As a developer, you can even remix forms you didn't design. Infact, it gives you complete themeability over everything. ------------------------------------------------------------------------ Sun, 28 Aug 2005 08:44:27 +0000 : adrian Attachment: http://drupal.org/files/issues/forms_presentation.pdf (78.63 KB) I hate the php filter .. here's the theme_ function .. i don't have time to escape the template right <?php function theme_node_form($form) { $output .= '<div class="node-form">'; $output .= '<div class="admin">'; $output .= '<div class="authored">'; $output .= form_render($form['author']); $output .= '</div>'; $output .= '<div class="options">'; $output .= form_render($form['options']); $output .= '</div>'; $output .= '</div>'; $output .= '<div class="standard">'; $output .= form_render($form_render); $output .= '</div>'; $output .= '</div>'; return $output; } ?> I am attaching my presentation from drupalcon in portland.It has all the examples, although stuff like the validation has changed slightly. ------------------------------------------------------------------------ Sun, 28 Aug 2005 16:53:48 +0000 : adrian Call for help : Code freeze for Drupal 4.7 is coming very quickly , and a lot of work is still needed to get this functionality in before the code freeze happens. As this patch is incredibly important (even if only considered from a security point of view), we need people to help us port all the forms in Drupal at the moment. If anyone is interested in helping, could you please contact me, so we can coordinate. ------------------------------------------------------------------------ Sun, 28 Aug 2005 22:19:40 +0000 : killes@www.drop.org You can assign me a module to conver as soon as my revisions patch is in core. I think I'd like to convert profile.module. ------------------------------------------------------------------------ Sun, 28 Aug 2005 23:41:06 +0000 : yogadex Regarding #24: Now I see why you keep the [printed] flag during node_render. Should this line: $output .= form_render($form_render); read: $output .= form_render($form); ??? (That is, I don't see where variable $form_render is coming from). Regarding #25: Must every form be converted in order to include this? Drupal 4.7 will still support the old forms API, yes? I see that this was discussed early in the thread. For those of us with custom modules life would be a lot easier if the old form api sticks around until say a 5.0 release. ------------------------------------------------------------------------ Mon, 29 Aug 2005 00:02:09 +0000 : killes@www.drop.org @yogadex: To those of us who roll out the actual security releases till 4am in the morning the disappearance of the old api with 4.7 will provide a lot more sleep. We win, backwards compatibility sucks. ------------------------------------------------------------------------ Mon, 29 Aug 2005 03:50:27 +0000 : adrian The old form api will remain in core for a maximum of one release. another 6-9 months to convert your modules is more than enough time... plus most of the new uber features (cck etc) are going to require you to upgrade to the new system. Meanwhile, new functionality like the install wizards are going to require you to use this form api. ------------------------------------------------------------------------ Mon, 29 Aug 2005 03:59:41 +0000 : adrian And regarding the form_render question .. it's not a variable, it's a small recursive function. the theme function is passed the entire form tree. The designer can choose to remix the form however he chooses. Since elements are nested, and for instance the fieldsets, are named .. you can use form_render($form['elementname']) , which will print that element, and set the printed flag so it won't be printed again. Like in the example where the author and options fieldsets are seperated out from the form and displayed seperately. form_render($form) in turn will print anything that hasn't been printed yet. if you wanted to seperate out the author field on the page, you could use form_render($form['author']['name']) , and it would print that element wherever, and when you then called form_render($form['author']) or form_render($form); , it would not be printed again. ------------------------------------------------------------------------ Mon, 29 Aug 2005 04:24:47 +0000 : yogadex @killes: I certainly don't want you losing sleep ;) I'm glad to hear the new and old will coexist for at least one release. (Unless there's a security issue - that's another story) @adrian: I understand the form_render() function and it's nifty. But there is a typo in both your attached presentation and #24 above, where "$form_render" is written and it should be "$form", if I am reading correctly. Thanks for explaining. ------------------------------------------------------------------------ Tue, 30 Aug 2005 21:14:37 +0000 : chx Attachment: http://drupal.org/files/issues/forms_1.patch (46.21 KB) I merged and rerolled against current HEAD. ------------------------------------------------------------------------ Wed, 31 Aug 2005 13:59:30 +0000 : m3avrck Minor include bug in the node.module patch: define('NODE_NEW_LIMIT', time() - 30 * 24 * 60 * 60); +include_once 'includes/form.inc'; that include_once is missing the './' part. ------------------------------------------------------------------------ Thu, 01 Sep 2005 00:25:22 +0000 : ax that second <?php +include_once 'includes/form.inc'; ?> shouldn't be changed but *removed*, because it is a duplicate of 8 lines above. ------------------------------------------------------------------------ Thu, 01 Sep 2005 00:31:52 +0000 : m3avrck ah yes, an even better catch than mine, touche! ------------------------------------------------------------------------ Thu, 01 Sep 2005 01:14:12 +0000 : chx Not so. The first should be removed 'cos it is hard to patch against the cvs id :) ------------------------------------------------------------------------ Thu, 01 Sep 2005 01:42:27 +0000 : ax allright, ok, of course, the first one. what would be even better would be a working patch. the ones attached (last one and before) don't include form.inc, generate some duplicate functions that result in "PHP Fatal errors: Cannot redeclare functions()" (system_elements(), theme_node_form()), and "PHP Warning: Call-time pass-by-reference has been deprecated - argument passed by value"s. chx: are you working on this? i would much like to help testing the new forms api and getting it into 4.7 because thats exactly what i need for a bigger project at work where we are considering using Drupal as base framework. the one showstopper is the extensability of Drupal forms ... thanks for all the work done up to now, anyway.