- Development - lists.drupal.org

Returned Mail: [drupal-devel] [task] Collapsible settings for all settings pages?
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: m3avrck <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [task] Collapsible settings for all settings pages? **************************************** Issue status update for http://drupal.org/node/30038 Post a follow up: http://drupal.org/project/comments/add/30038 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck -Status: active +Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/drupal_17.patch (9.17 KB) Rerolled against HEAD. Since it seems the FORM api is going to go in *after* the freeze, this should go in now, so before any of the forms are updated to the new API (so this change is accounted for later instead of having to do another patch :)) m3avrck Previous comments: ------------------------------------------------------------------------ Tue, 30 Aug 2005 17:37:22 +0000 : m3avrck Is there a reason collapsible fieldsets is used *only* on the main settings page? What about the users setting page? That is really long and would benefit. And the uploads setting page, as you add more roles, that page can get really long too. I think we should add this feature to all standard Drupal settings pages that are long like the settings page... it really helps and would make things look more consistent, instead of "why the heck is this on one page and not them all?". Need to get this into the next release as well to be consistent with the approach of this new feature. ------------------------------------------------------------------------ Tue, 30 Aug 2005 20:38:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/modules_0.patch (9.17 KB) I've gone through all of the settings pages for the included default Drupal modules and have applied the collapsible form fix. This really does make a *huge* difference... it makes the Settings interface more consistent and greatly improves readability. Let's get this one in! :) ------------------------------------------------------------------------ Tue, 30 Aug 2005 21:32:11 +0000 : Souvent22 Installed, and used the patched. Worked great. Much needed. :). +1 ------------------------------------------------------------------------ Tue, 30 Aug 2005 21:54:42 +0000 : chx -1 and +1 at the same time. +1 but only after the forms API is in -- it'd be better if we won't break the fastly evolving patch. ------------------------------------------------------------------------ Tue, 30 Aug 2005 22:28:25 +0000 : m3avrck Beat me to the post :) I just was reviewing the Forms API patch and realized this one is no longer applicable if that goes through. chx, can you include the fixes in this patch with the forms api? it just makes a few extra menus 'collapsible' which with the new API, requires even less code change, haha!

1 0

Returned Mail: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: m3avrck <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed **************************************** Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/drupal_18.patch (2.23 KB) Updated patch after talking with Thox and Uncloned to attach this event only to forms with a specific class (hence avoiding the problem of being on an edit page with other admin/search forms). m3avrck Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:06:44 +0000 : Junyor FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB) Updated JS styling and JS-killswitch after talking with Unconed on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck Just to build on Junyor's comment [6] this functionality is built into Opera 8 and it *doesn't* produce any errors. [6] http://drupal.org/#comment-44066 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB) Updated patch to fix possible problem of overwriting onsubmit event handler thanks to Thox on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:13:59 +0000 : webchick I tested this and can confirm that it works in *both* versions of Safari that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So looks like this code goes one better over the Blogger.com stuff, because their stuff doesn't workk in 1.3. Nice job! ;) ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck Well I'm gonna set this to commit then. Tested and working on IE, FF, Safari. Doesn't work or break Opera or Konqueror but not needed in these cases. Thox and Unconded have offered thoughts and I've modified code as needed. Doesn't seem like there is anything else left except a commit :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:13:48 +0000 : Bèr Kessels In general I dislike the feature. Konqueror somehow dies this, and that is the way it /should/ be. Browsers should take care of this, not the web app. But, since it is only konq. this patch has a good enough additional value :) I would like to see this patch tested with, tinyMCE and HTMLAREA, at least. For I am quite sure this will break these modules so bad that they are near unusable. Which brings me to the next point: using the textarea hook a simple module could take care of this. I would very very much prefer this living in a contrib, or even a core module. Just not "enforced" on me. And the last point: if this is for core, please use that textarea hook too. This is where that hook is for: extending the textareas.

1 0

Returned Mail: [drupal-devel] [task] New drupal forms api.
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: m3avrck <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [task] New drupal forms api. **************************************** 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: m3avrck Status: patch (code needs work) I agree with Ber and Souvent. I think we should all focus on our pet patches for the Drupal 4.7 freeze (which I think should be Friday give us that one day ;). After that, we should have a "sub-freeze" where *only* FORM related API patches go in, set this for 3-4 days after the 4.7 freeze. Then, once that is complete, *then* get back into the regular bug/performance/usability freeze over all. m3avrck 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(a)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(a)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. ------------------------------------------------------------------------ Thu, 01 Sep 2005 02:38:08 +0000 : jvandyk 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. ------------------------------------------------------------------------ Thu, 01 Sep 2005 02:43:01 +0000 : m3avrck Not to nitpick, but in node.module, the patch has +include_once 'includes/form.inc'; but should read +include_once './includes/form.inc'; for consistency and performance :) ------------------------------------------------------------------------ Thu, 01 Sep 2005 03:24:16 +0000 : webchick I don't know if this is just the Drupal newbie in me talking, but I am getting quite a few errors with this patch (Drupal HEAD, up-to-date as of about 5 minutes ago). I've uploaded the form.inc from #19 and applied the forms_2.patch from #38, and I'm getting errors like the following: *admin/settings:* warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 511. *admin/comment/configure, admin/settings/user*: warning: Missing argument 2 for system_settings_form() in $DRUPAL_ROOT/modules/system.module on line 733. *node/add/[anything]*: warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 406. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 403. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 411. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 406. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 403. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 411. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 406. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 403. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 411. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 406. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 403. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 411. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 406. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 403. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 411. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 435. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 448. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 435. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 448. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 435. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 448. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 435. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 448. warning: Invalid argument supplied for foreach() in $DRUPAL_ROOT/includes/form.inc on line 435. warning: Cannot use a scalar value as an array in $DRUPAL_ROOT/includes/form.inc on line 448. --- Any ideas? ------------------------------------------------------------------------ Thu, 01 Sep 2005 03:35:44 +0000 : jvandyk That's why I said "mostly working." :) m3avrck, sorry, I diffed against the copy where I hadn't corrected the ./ thing. ------------------------------------------------------------------------ Thu, 01 Sep 2005 09:47:05 +0000 : adrian Good morning guys. I am looking at those errors now, and i will upload an updated forms.inc in a short while. ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:46:02 +0000 : adrian Attachment: http://drupal.org/files/issues/form_2.inc (26.76 KB) Here is an updated patch and includes/form.inc. Changes : 1 ) Introduce a global $form_variables array, which works exactly like $_POST['edit'] used to, except the values in it are filtered through the form system and only values that have form elements will be in it. Each form being processed on a page resets this array, so they won't interfere with each other. 2) introduce a form_id hidden that gets submitted with the page. This means it can intelligently decide which form to process. 3) Fix the bug above. I wrapped the offending lines in if is_array() , HOWEVER .. at the point I call that code I am fairly certain I am only working on arrays, hence I need to look it over more 4) fixed the user login form on /user. The user login block still doesn't work 5) Added a default css class (and some temporary css in drupal.css) for everything rendered with the form api. All other forms will have a thick red border if they have not been ported. 6) fixed a bug with element_info where it was setting the wrong defaults and then messing up the first form element because of it. 7) finished the filter module 'filter_format' element. Filter forms are printing now. 8) Added the 'weight' element. Could be cool to theme this with something ajaxy, right ? 9) Fixed some bugs with admin/system 10) Updated the following modules : Book, Page, Story, Blog, Node, Taxonomy and a fixed user form. 11) Moved the inclusion of form.inc to it's rightful place in drupal_bootstrap_full. 12) Added a bunch of comments to places. We will still have to go through all the comments and make sure they all 'parse' =) The node form is not actually working or validated at present, and if someone could take a look at that , I'd be much obliged. It's important to get used to the new (strict) workflow for forms. It generally means having to shift some code around, and it makes it harder to write 'bad code'. ------------------------------------------------------------------------ Thu, 01 Sep 2005 23:05:56 +0000 : adrian Attachment: http://drupal.org/files/issues/forms_3.patch (54.13 KB) here's the patch ------------------------------------------------------------------------ Fri, 02 Sep 2005 05:48:44 +0000 : Dries /introduce a form_id hidden that gets submitted with the page. This means it can intelligently decide which form to process./ Does this mean people could change the ID before they submit the form? I'm not sure that is a good idea. ------------------------------------------------------------------------ Fri, 02 Sep 2005 10:04:21 +0000 : adrian The access check is tied to the page, not to the form. If the page has 2 forms on the page, this check is to see which of the two he submitted. Changing the form_id in the hidden will do nothing but cause the form not to execute, at all. ------------------------------------------------------------------------ Fri, 02 Sep 2005 14:04:45 +0000 : m3avrck Running this patch I'm getting this error Fatal error: _drupal_bootstrap_full() [function.require]: Failed opening required './includes/form.inc' Didn't see the file anywhere in the patch, unless I missed something... ------------------------------------------------------------------------ Fri, 02 Sep 2005 14:44:22 +0000 : adrian drupal.org renames the files to avoid conflicts. it's attached as form_2.inc ------------------------------------------------------------------------ Sat, 03 Sep 2005 03:06:21 +0000 : asimmonds Should there be "return_value" and "delta" property constants defined in form.inc? ------------------------------------------------------------------------ Sat, 03 Sep 2005 06:32:57 +0000 : adrian Attachment: http://drupal.org/files/issues/form_3.inc (27.33 KB) Well spotted. Added those defines. ------------------------------------------------------------------------ Sat, 03 Sep 2005 11:53:53 +0000 : nicopepe Attachment: http://drupal.org/files/issues/modif_Form_Inc.php (2.52 KB) Hello I am a newsbe and i am vey concern about the form project. I want first to congratulaed for all this nice work. I propose this code in the attached file. Tell me what you think, and i'll finished it. ------------------------------------------------------------------------ Sun, 04 Sep 2005 14:57:30 +0000 : nicopepe Hello again, I see no reaction to my proposition. Maybe not interresting code ? My aim with that code is : - Open all the possibilities of attribute in an input balise. - Not multiply the number of constant (The key of an element is an attribute name, define by the programmer) - Check within drupal_attributes function if these attributes are correct in an INPUT Balise. - Include a javascript code insertion for form client validation. In fact i would like to create later on, a module with generic javascript for client validation (Date checking, required fields, ...). I have this in mind when i proposed this code. Any answer this time ? ------------------------------------------------------------------------ Sun, 04 Sep 2005 15:11:41 +0000 : clydefrog nicopepe, it is difficult to tell what your changes are because you have not submitted a patch. Take a look at http://drupal.org/patch. Your code might get a better response that way. ------------------------------------------------------------------------ Sun, 04 Sep 2005 18:25:23 +0000 : Dries As an aside, it might be worth looking into the XForms [1]. They have a short introduction for HTML authors [2] in case you don't feel like reading the entire specification. A friend of mine is a member of the XForms working group and told me someone was working on a JavaScript implementation of XForms. More importantly, XForms have (client-side) validation build-in. For example, to specify a valid range you use: <range ref="volume" start="1" end="10" step="0.5"/> Similarly, XForms have the ability to state that a value must be supplied before the form is submitted, have a notion of typed values (eg. you can specify that you expect an URL, integer or date to be entered). XForms also include stuff to introduce wizard-like behavior of filling in a form in stages, include functionality to dynamically add fields (eg. like when you run out of textfields for poll questions), etc. A lot of the stuff we have been talking about. :-) It might be useful to check if our developer API would map gracefully on XForms. It would impose that we are XForms-ready (future proof). It's especially interesting because Mozilla is working on a XForms implementation [3]. (XForms 1.0 support will be included in Firefox 1.1. You can test it using the nightly Firefox builds.) [1] http://www.w3.org/MarkUp/Forms/ [2] http://www.w3.org/MarkUp/Forms/2003/xforms-for-html-authors [3] http://www.mozilla.org/projects/xforms/ ------------------------------------------------------------------------ Mon, 05 Sep 2005 11:15:16 +0000 : nicopepe As a newsbe, i have not install any patch module Yet. So i will do it. For Dries, i will have a look on this Xforms projet, but i just wonder if it is MS compatible (80% of market) ? Maybe before adding this code I should have write my ideas. I think this new form API should consider having an easy way to perform database update as well as new records adding. So in my view, 2 specials “events” are needed : - call an user defined procedure BEFORE the Form is Loaded (non existing yet ?) to read database and set value to the form. - call an user defined procedure AFTER the Form is validate (drupal_execute_form)to write to the database update or new record. I agree with Adrian to use arrays. They allowed flexibility. I think using $form and $element arrays with others keys than theses defined with property constants is a good way. Property constants are used for regular Drupal core functionality, and user defined keys values used for extra functionalities. This means that the $form array should have a well documented structure to be able to extend it. I see here three examples: - Use all JS events (as my code suggest) for client validation. examples : $form['onSubmit'] = "js function name or code..."; $element['onkeydown'] = "js function name or code..."; - Create an $element[‘NonValidationMessage’] Key used in drupal_validate_form user function to return to the user when the validation of an element is not OK (This message could also be used in JS in client validation). - Possibility for a programmer to input in the $element array the name of the database field and database table to store the input value in case of correct validation. Exemple : $form['tablename'] = "mytable"; $form['idValue'] = "value"; $form['idName'] = "myid"; $element['Databasefieldname'] = "myfieldname"; Then create an function to create automatic SELECT STATMENT to read the value before display the form ( When we need to modify value stored in a database) or INPUT SQL statement after validation, I hope i am clear with my ideas. Maybe some of them could be used already in the core. ------------------------------------------------------------------------ Mon, 05 Sep 2005 11:47:08 +0000 : Dries nicopepe: I'm not saying we should implement XForms at this point. I meant to say that (i) XForms looks like an emerging standard and that (ii) the XForms API tries to solve the same problems we are trying to solve with the new form API. It is worth investigating XForms. Being able to generate XForms-compliant code with minimal changes is a nice extra, but certainly not a must at this point. Hope that clarifies my comment. ------------------------------------------------------------------------ Mon, 05 Sep 2005 12:01:28 +0000 : Junyor Dries: It'd probably be more interesting to look at Web Forms 2: http://www.whatwg.org/specs/web-forms/current-work/. It's geared toward the Web, whereas XForms isn't. Web Forms 2 is also more likely to see native implementations. ------------------------------------------------------------------------ Mon, 05 Sep 2005 12:33:38 +0000 : killes(a)www.drop.org Dries: In my book we are trying to solve a securoty issue woth this patch: the case of inserting bogus fields into our html forms. Client side validation is noce from a usability pov but we are trying to do server side. ------------------------------------------------------------------------ Mon, 05 Sep 2005 14:02:58 +0000 : Dries Gerhard: it's obvious that client-side validation is no replacement for server-side validation. :-) Anyway, instead of looking years ahead, let's focus on the patch at hand. ------------------------------------------------------------------------ Mon, 05 Sep 2005 15:06:27 +0000 : nicopepe Client side validation will never replace server side validation for security reason ! I do understand that client side is not yet a priority, but please let doors open for module developper if they want to implement Client side validation in their module. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:55:57 +0000 : adrian Attachment: http://drupal.org/files/issues/forms.tar.bz2 (0 bytes) Here is an updated, and more complete version of this patch. I had a bit of a SNAFU with the multiples code, and needed to refactor it to work properly. Changes in this version : 1) Introduced a 'process' property. This is used by the radios and checkboxes elements, to call 'expand_radios' and 'expand_checkboxes' functions which turn the single element into a set of elements. 2) admin/themes screen done. 3) deprecated fix_checkboxes, since it was no longer needed. Checkboxes and radios work perfectly now ( they were broken previously). 4) The way to access the form value is through a global $form_values array, that is set up to the structure of the form. It's no longer flattened, and it works perfectly. 5) Introduced an array_walk_recursive function (copied from PHPCompat in PEAR) that allows you to step through the values and do something with each key. (admin/settings uses this property). 6) Moved the old form api into a legacy.inc 7) Did a bunch more forms. In this tarball you will find the 2 new files (legacy.inc and form.inc) and the current patch. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:57:30 +0000 : adrian Attachment: http://drupal.org/files/issues/Forms.rtf (14.02 KB) Here's the status document of all the forms (rtf export from omnioutliner) It's incomplete, as I haven't finished cataloging all the forms yet. ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:01:25 +0000 : m3avrck No patch in that zip :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:54:08 +0000 : m3avrck Also, adrian, is there a way to add events or set variables in javascript, if the submit button on a form has been clicked? Need this functionality for a patch about to (hopefully!) make its way into HEAD soon. ------------------------------------------------------------------------ Mon, 12 Sep 2005 20:51:26 +0000 : adrian Attachment: http://drupal.org/files/issues/forms.patch.txt (108.18 KB) I'm sure it's possible, but I think we prefer attaching events and the like onto forms using the id attribute. here's the patch file. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:07:59 +0000 : adrian Attachment: http://drupal.org/files/issues/Forms_0.rtf (22.47 KB) Here's a complete list of the forms in drupal that still need changing. Everything that has already been done is not on this list (ie: all the node modules, and the like) Some of the forms mix normal output, and form output in the same page, and switch out depending on certain requirments (the aggregator_list_page was an example of this), making it a fairly intensive process to seperate the two pieces of functionality into clean code. If something is rated simple, it means it only involves changing the functions to arrays, and either returning the array, or returning the final form using drupal_get_form('form_id', $form); If we want any chance of getting this into core before 4.7, I am going to need some volunteers to handle tackling these forms for me. Once again, I am looking for a few good men (where men == female OR men == male .. obviously. =) ). I am going to be concentrating on the really complex forms, and the theme('confirm') form, which is required by a fair amount of things. ------------------------------------------------------------------------ Tue, 13 Sep 2005 03:38:20 +0000 : Jeremy(a)kerneltrap.org I thought I'd give this patch a quick try, but ran into a couple problems. First is very minor, it makes some changes to settings.php that it shouldn't -- something to clean up before the final version. Second, it adds includes for form.inc and legacy.inc, but the files themselves are not actually included in the patch. ------------------------------------------------------------------------ Tue, 13 Sep 2005 11:53:05 +0000 : adrian They are in the tarball i uploaded earlier. ------------------------------------------------------------------------ Tue, 13 Sep 2005 12:07:31 +0000 : Jeremy(a)kerneltrap.org I tried that last night too, but the earlier tarball was 0 bytes and contained no files: Attachment: forms.tar.bz2 (0 bytes) ------------------------------------------------------------------------ Tue, 13 Sep 2005 12:22:52 +0000 : adrian Attachment: http://drupal.org/files/issues/forms.tar_0.bz2 (27.82 KB) That is the weirdest thing .. locally : Benton:~/dev/forms adrian$ ls -l forms.tar.bz2 -rw-r--r-- 1 adrian adrian 28489 Sep 12 15:30 forms.tar.bz2 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:03:17 +0000 : adrian Attachment: http://drupal.org/files/issues/Forms.html (10.02 KB) Html version of the status list. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:12:22 +0000 : Dries Guys, please take a look at the status page. If we want the new form API in Drupal 4.7, we'll have to work together pretty much non-stop to complete it! Care to help? Pick an item, migrate to the new form API and upload a patch. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:26:09 +0000 : Bèr Kessels Can we not exclude this from the code freeze? I mean that we now (last days) focus on all these outstanding patches for 4.7. and then have acollaborative effort on this forms patch. That will at least give us the benefit that we can aim on a stable target. Instead of junting on a still changing core. From what I see a lot of people are focussing very hard at their babies in the queue, to try to push them into 4.7 (that counts for me anyway) So they will not spend any time on the form api yet. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:41:45 +0000 : webchick For what it's worth, I agree with Bèr. I've been picking away at comment.module to make it compatible with the forms API, but it's very difficult since the patch needed to enact all these changes differs from day to day (today I applied the .tar'ed version and still am getting errors with a few hunks which I need to troubleshoot first in order to make sure my changes aren't causing errors). Once things are more "settled" with the rest of core, it seems like it would be much easier to make a more "stable" patch as a starting point, which would thus ease efforts on the part of the rest of us to get the changes made. This patch represents a HUGE change, and I worry about the idea of trying cramming it in at the last minute. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:54:29 +0000 : Souvent22 I totally agree with the above comments. With so many chagnes happening so fast, we wake up to a new core each morning, and must make changes accordingly. I don't know what the core will get 'stable' enough before the freeze to really hack through what needs to be done in an efficient manner. We all seem to agree this *needs* to happen, but perhaps a better plan of *'how'* it should happen since it is such a big change would make things easier, more stable, and not make it seem like we're trying to cram a square block through a round hole.

1 0

Returned Mail: [drupal-devel] [feature] Block query consolidation
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from I1uv4t4r <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: I1uv4t4r <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [feature] Block query consolidation **************************************** Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: I1uv4t4r -Status: patch (ready to be committed) +Status: patch (code needs work) Attachment: http://drupal.org/files/issues/block.module_6.patch (966 bytes) First I want to say that I am astonished that those patches have made it into CVS. Both the database and block.module patches don't work for me and cause significant errors. Firstly, a primary key of module, delta and theme makes MySQL throw up a 'key too long' error. Secondly, in block.module an array is fetched from the database and then it is used as an object, and the blocks are added to the region the function is called for. You'll understand when you see my patch. I must say that my patch is not tested thouroughly, but I believe it impossible that the patches above are even tested at all, but tell me if I'm wrong. The patch I provide is solely for block.module by the way. I1uv4t4r Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck Exactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:27:32 +0000 : Dries Thanks for the measurements! Patch committed to HEAD. Thanks. ------------------------------------------------------------------------ Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds With this patch committed, and updating my HEAD install with update.php, I get the error "Duplicate entry 'user-0' for key 1" from mysql. I have two themes configured and there is a primary key (module,delta) clash between the two themes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:39:43 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB) Eeek!!! Forgot to test with multiple themes, this patches fixes the key definition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:41:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_4.patch (1.96 KB) Noticed a typo in the PGSQL. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:00:19 +0000 : mathias I can recreate this bug when running update.php but I question the necessity of this index in the first place. I couldn't find one SELECT query in block.module that benefit from it. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:18:07 +0000 : Allie Micka For what it's worth, the benchmarks I submitted did not include the key/index patch, they only included the query consolidation patch. The performance impacts of the patch that hijacked this issue are unknown. I'd suggest moving this issue back to http://drupal.org/node/27157 , where you can assess performance impacts and the back out or correct that patch based on the results. Alternatively, continue using this issue but supply a few benchmarks to justify adding and/or removing the block table indexes. Thanks! ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:56:32 +0000 : m3avrck Just chatted with mathias on IRC. I think there was some slight confusion and I apologize for this. The other issue is related to this issue indeed as it affects the block query and that is why Dries above mentioned to incorporate that fix into this one. Then both of these were rolled into the latest HEAD. However, a small oversight on my part, the key wasn't unique in all situations, and this above patch rectifies this situation. Now, in terms of performance, the key doesn't affect performance in any measurable way. Rather, it adds data integrity and structure to the table, and enhances other *unknown and future* queries. I chatted with a knowledgeable fellow on #mysql a while about this table and it was determined that a key *was* beneficial, even if any performance gains were not. In hindsight, maybe these patches shouldn't have been put together, but as such, they were, and this above patches fixes this issue and brings us back up to speed. So basically this patch "consolidates block queries *and* adds data integrity to the block table". :)

1 0

Returned Mail: [drupal-devel] [task] Extend db_query()
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from killes <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: killes <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [task] Extend db_query() **************************************** Issue status update for http://drupal.org/node/17656 Post a follow up: http://drupal.org/project/comments/add/17656 Project: Drupal Version: cvs Component: database system Category: tasks Priority: normal Assigned to: killes(a)www.drop.org Reported by: killes(a)www.drop.org Updated by: killes(a)www.drop.org Status: patch (code needs review) Attachment: http://drupal.org/files/issues/db_query_3.patch (2.85 KB) Kjartan prefers this version. I also added some comment. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ Mon, 21 Feb 2005 12:48:30 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db-query.patch (2.16 KB) We should make our database abstraction layer more robust and ensure that module authors can use it without string manipulations inside the query. Several queries use implode() to get their arguments into the query. This is undesirable as we rely on the module author to check the keys and values of such arrays for exploitation attempts. I have created the attached patch which shouldbe able to allow us to not use implode anymore. A minor problem is that all inserted values will be treated as strings. This might be a problem with PostgreSQL at least. However, the same strategy is already used in Drupal core without any complaints I know of. Summary: This patch will alow us to simplify some code in node.module, user.module, taxonomy.module and probably others. ------------------------------------------------------------------------ Mon, 21 Feb 2005 17:03:58 +0000 : killes(a)www.drop.org It's a patch. ------------------------------------------------------------------------ Mon, 21 Feb 2005 17:19:13 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db-query_0.patch (2.07 KB) Squeezed out two lines of code after consultation with Karoly. Adds only 10 loc (plus some docs). ------------------------------------------------------------------------ Mon, 21 Feb 2005 17:23:08 +0000 : chx Do I need to say +1? ------------------------------------------------------------------------ Thu, 03 Mar 2005 00:15:10 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db-query_1.patch (2.69 KB) After some discussion with Adrian at Drupal Con we found out that we do not know why node_save currently works with pgsql. It currently assumes that all db columns are strings. It seems to work but we should not rely on it. Here is a patch that checks for the type of field that is inserted. It needs testing. ------------------------------------------------------------------------ Tue, 26 Jul 2005 01:17:04 +0000 : drumm +1 for making this into an API. I've seen too many hacked together query builders in Drupal and Contrib. I have not tested. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:30:29 +0000 : Bèr Kessels untested. a big +1 for the feature ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:41:52 +0000 : killes(a)www.drop.org the patch still applies. the new patch here updates node_save to use it. Untested. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:42:24 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node-%a.patch (926 bytes) the patch still applies. the new patch here updates node_save to use it. Untested. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:45:17 +0000 : Cvbge "After some discussion with Adrian at Drupal Con we found out that we do not know why node_save currently works with pgsql. " Well, it does not work. Real life exampless of not-working include flexinode (my experience) and forum module (as someone reported). Probably also others. The bug occurs when a normal user (but with necessarily rights) adds a node and he has no controls (the 'moderated', 'sticky', 'published' etc). The, in node_load() $node->sticky, $node->moderated (and maybe others) are set to FALSE (or TRUE, but in this case it works). When doing printf("%s", FALSE) the FALSE is change to empty string. The sticky and moderated db fields are numeric and postgresql do not accept '' (empty string) as a value of integer type. The result is for example such error: warning: pg_query(): Query failed: ERROR: invalid input syntax for integer: "" in ..../includes/database.pgsql.inc on line 45. user error: query: INSERT INTO node (title, uid, type, teaser, status, moderate, promote, sticky, body, comment, created, changed, nid) VALUES('xx', '2', 'flexinode-1', '<div class="flexinode-body flexinode-1"><div class="flexinode-image-3"><div class="form-item"> <label>Zdjęcie:</label><br /> <img alt="xx" src="..../pliki/" /><br />Get original file (28KB) [1] </div> </div></div>', '1', '', '1', '', '<div class="flexinode-body flexinode-1"><div class="flexinode-image-3"><div class="form-item"> <label>Zdjęcie:</label><br /> <img alt="xx" src="..../pliki/" /><br />Get original file (28KB) [2] </div> < in ..../includes/database.pgsql.inc on line 62. [1] http://drupal.org/..../pliki//tmp/male.jpg [2] http://drupal.org/..../pliki//tmp/male.jpg ------------------------------------------------------------------------ Wed, 27 Jul 2005 13:03:14 +0000 : Cvbge Attachment: http://drupal.org/files/issues/node.module_5.diff (500 bytes) Here's a quick fix for 4.6 ------------------------------------------------------------------------ Wed, 27 Jul 2005 13:10:54 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node-a.patch (1.8 KB) the last patch wasn't good. ------------------------------------------------------------------------ Mon, 08 Aug 2005 08:19:14 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_api_taxonomy.patch (3.5 KB) Here is an additional patch for taxonomy.module it removes the two custom functions that are used for inserts and updates. Needs testing. @Cvbge: shoduld we have a better test than just is_numeric in _db_argument_type to fix the pgsql problems? We could also try to pass $value by reference and cast it to int. this function is only used for updates/inserts so we can afford an additional check. ------------------------------------------------------------------------ Mon, 08 Aug 2005 08:37:25 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query.patch (2.74 KB) Here's a revised version fo the original patch. _db_argument_type now gets the $value argument by reference and we check for bools too. numerics and bools are cast to int. ------------------------------------------------------------------------ Mon, 08 Aug 2005 10:34:36 +0000 : Bèr Kessels I cannot say anything about performance for I do not know how to benchmark this. But I tried it, And rewrote some queries in a custom module, to use it, and like it a lot. The code becomes cleaner, and better readable. But above all, i see this idea going into a very interesting and good direction: that of 'more' query builders in core'. a +1! ------------------------------------------------------------------------ Mon, 08 Aug 2005 10:51:14 +0000 : Cvbge I'll be talking about postgres db only. The patch won't work as expected in some cases. Let's say we have a text/char/other character column and we want to insert a text that looks like a number. The _db_argument_type() will recognize the value as a numeric/number etc and will use $key = %d format. Even not considering printf() numeric conversion this can lead to incorrect value inserted into db. Examples of such problematic texts are: 01 (leading 0), 2e2 (scientific notation), probably also text with trailing/leading white spaces. When inserting such data into db without quotes the postgres db will think this is a number and will change 010 to 10, 2e2 to 200 . Here is an example: piotr=> CREATE TABLE t(c CHAR(10)); piotr=> INSERT INTO t VALUES (01); piotr=> INSERT INTO t VALUES (2e2); piotr=> INSERT INTO t VALUES (-01); piotr=> INSERT INTO t VALUES (000); piotr=> SELECT * from t; c ------------ 1 200 -1 0 (4 rows) I think it's safe to use '%s' everywhere. I could not find it writtent directly in postgres documentation (although I remember reading it somewhere some time ago). Postgres will convert the string to correct integer/numeric/bool value. ------------------------------------------------------------------------ Sat, 13 Aug 2005 01:14:41 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query_0.patch (2.69 KB) Ok, the patch now does only distinguish between integerss and the rest. This is needed to decide whether we need quotes. ------------------------------------------------------------------------ Sat, 13 Aug 2005 09:40:59 +0000 : Cvbge I'm sorry, but I see no reason for this integer check. Can you explain why there is a need to differentiate between integers and other types? The only problem I see is with converting bool(FALSE). This value is converted to empty string ('') and when trying to insert it into numeric, float, bool, date (probably any column that is not text type) etc you get error. One solution would be to check for bool(FALSE) and convert it to 0. This would work for numeric-like and bool fields. But it would not work for DATA columns (would produce error). Also, entering '0' into text column might not be what the author wanted. But this is more a problem with the coder - if you have integer/date etc field, insert integer/date/etc data type, not bool! Side note: the integer-version, the one without '', will be used only for integers (i.e. ..., -2, -1, 0, 1, 2, ... see http://php.net/manual/en/language.types.integer.php) ------------------------------------------------------------------------ Sat, 13 Aug 2005 09:43:19 +0000 : Cvbge "DATA" should be "DATE" ------------------------------------------------------------------------ Sun, 14 Aug 2005 00:53:34 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query_1.patch (2.71 KB) Ok, another attempt. There is indeed no real reason to use %d for integers, they work fine as strings and are converted by sprintf to strings independend of %d or %s. But it is the Right Thing(tm), so I kept it. Booleans, however, need the %d formatter in order to be converted to 0 and 1. TRUE to 1 always works, but FALSE to 0 only works with %d. All tests on php 4.3.x. ------------------------------------------------------------------------ Thu, 18 Aug 2005 21:46:04 +0000 : Cvbge The part I was objecting previously looks ok now. I haven't tested the code though. I still think it'd be nice to change php NULL to real SQL NULLs, but I don't have how to do it (at least not with current approach). The best would be if there was (s)printf flag that would just ignore it's argument... ------------------------------------------------------------------------ Sat, 27 Aug 2005 19:36:04 +0000 : Thomas Ilsche Attachment: http://drupal.org/files/issues/node_save.patch (907 bytes) +1, tested Runs nicely but maybe NULL and floats might need special treatment. I have attached an INCOMPLETE patch for node_save using the new feature that also fixes the following issue. ------------------------------------------------------------------------ Tue, 30 Aug 2005 18:13:48 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node_save_0.patch (3.65 KB) Here is an updated patch that takes a lot of the code I added to node_save out again (approx 20 loc). it also fixes a docs glitch in that function. Needs testing and is to be used with the patch from http://drupal.org/node/17656#comment-39222 ------------------------------------------------------------------------ Sun, 04 Sep 2005 18:13:36 +0000 : Cvbge A remark about NULL values: in this patch they are treated as integers and thus allways changed to '0'. In existing code the '' was INSERTed into text-like columns (%s) if didn't check if the variable was set (or did not care). If they now use %a and still do not check for NULLs the '0' will be inserted which migh or might not create problems (it won't matter for if()s, but maybe for other uses?) ------------------------------------------------------------------------ Sun, 04 Sep 2005 19:50:58 +0000 : killes(a)www.drop.org Cvbge: AFAIK we don't use NULL values inside the DB in Drupal core. ------------------------------------------------------------------------ Sun, 04 Sep 2005 20:56:41 +0000 : Cvbge You're probably right, then 1. not checking if a variable is set is a bug (and I've already filled a bug for one of such bugs, can't find it - sticky, moderated etc. fields were not set at all when not selected when submitting a post) 2. There's already a lot of DEFAULT NULL in the database schema... ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:03:09 +0000 : Cvbge Hello, NULLs again. Previously I wrote that NULLs are treated as integers and passed with %d and converted to 0. This is of course not true [were I sick when writting it?]. They are treated as strings and converted to '' (empty string) This will create similar problems as FALSE. Maybe NULL should be treated as %d (and converted to 0) after all? ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:06:30 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query_2.patch (2.72 KB) Ok, I've included is_null() in the condition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:31:55 +0000 : Cvbge ok, last thing (really ;)): maybe add some information to docs saying that null is converted to '0' (not to '') ? :)

1 0

Returned Mail: [drupal-devel] [bug] node revisions should only be viewable by admins
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from killes <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: killes <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [bug] node revisions should only be viewable by admins **************************************** Issue status update for http://drupal.org/node/30098 Post a follow up: http://drupal.org/project/comments/add/30098 Project: Drupal Version: cvs Component: node system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: killes(a)www.drop.org Updated by: killes(a)www.drop.org Status: patch (code needs review) Attachment: http://drupal.org/files/issues/node_rev_4.patch (11.32 KB) Oops, I confused a permission. it needs to be node_access('update', $node), note 'edit'. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ Wed, 31 Aug 2005 13:36:19 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node_rev.patch (1.27 KB) With or without the revisions patch, every user who can access content can access old revisions. I think this is a bug because why would you need to see old revisions if you cannot change them or make them the current revision? The attached patch fixes this and lets only users with "administer nodes" permission see old revs as you need this permission to change revisiosn to be the current one. One might make a case for introducing new "view revisions" and "set revisions" perms. You woud need those if you wanted to mimic a wiki with Drupal. ------------------------------------------------------------------------ Wed, 31 Aug 2005 13:55:14 +0000 : jakeg +1 I would also like to see the extra permissions added if possible: 'view node revisions' - just for viewing node revisions 'administer node revisions' - to delete node revisions or set them as default revision (i.e. current revision) for a node I think this patch is important because e.g. if a revision is made because sensitive information was included in a past revision, such as a phone number, plain text email address (someone stupid didn't know about spammers or the contact form) then these should definitely NOT be accessible, and it would often be desirable to create a new revision to a node rather than to edit the current copy without creating a revision. ------------------------------------------------------------------------ Wed, 31 Aug 2005 13:55:28 +0000 : Morbus Iff I disagree. I see revisions as a means of updating a document for the users. If a user has read the document, then sees that a revision has been made, I'd want to provide him with some sort of diff that shows him exactly what, as opposed to making him read the whole blasted thing over and over again. In essence, I want a a wiki diff between revisions (for example) [1]. Putting this permission in place would restrict my ability to do that. [1] http://gamegrene.com/wiki/?title=WhereIsWhere&curid=927&diff=0&oldid=0&rcid… ------------------------------------------------------------------------ Wed, 31 Aug 2005 13:57:15 +0000 : Morbus Iff Note to self: actually read the whole report before commenting. -1 to JUST this patch. +1 to view/set revisions. ------------------------------------------------------------------------ Wed, 31 Aug 2005 13:59:13 +0000 : jakeg good point #2 morbis, but the revisions tab isn't accessible to normal users anyway... they only get to see the revisions if they know the URL to get there (?revision=x in 4.6; /revisions/x in head) but i agree that its definitely better with the extra permissions ------------------------------------------------------------------------ Thu, 01 Sep 2005 19:39:54 +0000 : robertDouglass Please don't lump it in with administer nodes, please make a new permission. ------------------------------------------------------------------------ Thu, 01 Sep 2005 21:02:15 +0000 : Boris Mann Yes, please make this a separate permission. In actuality, the use cases for viewing revisions probably needs a permission *per node type*. E.g. a site uses "story" for front page content. Editors some times revise stories, but don't want those revisions public. The same site uses book pages like a wiki. It gives all users create/edit/maintain book privileges, and additionally wants them to be able to see revisions. So, if we don't enable viewing revisions for node type, I would hope for a way to override this at the node type/module level -- so a custom wiki.module could automatically add a "view revisions" permission. ------------------------------------------------------------------------ Thu, 01 Sep 2005 23:55:46 +0000 : killes(a)www.drop.org Boris, what you propose makes sense to me. I am however waiting for feedback from Dries before I re-roll this patch. if we introduce this new permissions, maybe we should factor the revisions out into their own module? ------------------------------------------------------------------------ Fri, 02 Sep 2005 01:37:34 +0000 : Boris Mann Revisions as a separate module might very well make more sense. Sites can choose to run with them enabled or not. Might make it easier to eventually do file versioning if we can hook into a module. ------------------------------------------------------------------------ Fri, 02 Sep 2005 05:46:30 +0000 : Dries +1 for the permission(s), -1 for moving the revisions to a new module at this point. ------------------------------------------------------------------------ Fri, 02 Sep 2005 13:08:21 +0000 : killes(a)www.drop.org Dries, thanks for the feedback. I will prepare such a patch. Thanks for saving my time, too. ;) Boris, we already do some sort of file revisions. A file that is attached to one revision can be deleted from another one, but will still be available with the original revision. Only if we delete a node completely, the file will actually deleted (or you delete it from every revision). ------------------------------------------------------------------------ Fri, 02 Sep 2005 15:18:03 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node_rev_0.patch (7.69 KB) Ok, here is a patch which implements edit and view permissions per node type. "admin nodes" is still valid as well. Needs testing. ------------------------------------------------------------------------ Fri, 02 Sep 2005 15:23:42 +0000 : killes(a)www.drop.org NB: I'd appreciate a review from wiki users. I've implemented "view" as access to the revision overview page and the individual revs and ""edit" as "change current revision" + "delete revision". I guess the latter is a bit dangerous. ------------------------------------------------------------------------ Fri, 02 Sep 2005 22:20:01 +0000 : Morbus Iff I haven't tested the patch, but "delete revision" concerns me - it's the style of a wiki to NEVER delete a previous version of a document - to always have a permanent record of its manifestations. Annoyingly enough, I woudn't +1 these new permissions until "delete" became its own perm as well, separate from edit. ------------------------------------------------------------------------ Sat, 03 Sep 2005 16:20:00 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node_rev_1.patch (7.76 KB) Expected this. ;) I've renamed "edit revisions" to "revert revisions" because that is what you can do with that permission. I did not introduce a "delete revs" permission but moved that permission back to "administer nodes". ------------------------------------------------------------------------ Sat, 03 Sep 2005 16:45:16 +0000 : Morbus Iff Sounds good to me! ------------------------------------------------------------------------ Sat, 03 Sep 2005 19:50:24 +0000 : mathias Having node module set revision permissions for all other node modules contradicts the interface of the permissions screen. The current interface suggests that if I want to set revision permissions for books, I would look underneath book heading, not node. Interface issues aside, if node module is going to set revision permissions for all other node mods why stop there? Why not "create $type", "edit own $type", etc? ------------------------------------------------------------------------ Sat, 03 Sep 2005 22:38:43 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node_1.module (72.61 KB) @matt: Yeah, you are right abiut that screen. No idea what to do, though. I don't think that we shoudl create permissions by default if we cannot enforce their use. For the rev perms we can that. I've attached a patched node module for testers coming from http://drupal.org/node/30329 ------------------------------------------------------------------------ Mon, 05 Sep 2005 00:49:06 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node_rev_2.patch (10.69 KB) Here's a new patch that adds some node_access checks and reorganizes the code a bit. node_page was already too spaghetti so I made a new menu callback for revisions related stuff. ------------------------------------------------------------------------ Mon, 05 Sep 2005 00:50:17 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node_2.module (73.1 KB) and the patched module ------------------------------------------------------------------------ Mon, 05 Sep 2005 20:22:34 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node_rev_3.patch (11.31 KB) Ok, here is a new patch whihc fixes the permission bug and also creates new revisions when reverting. ------------------------------------------------------------------------ Mon, 05 Sep 2005 20:24:52 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node_3.module (73.36 KB) And the module ------------------------------------------------------------------------ Tue, 06 Sep 2005 14:57:52 +0000 : cel4145 Using the latest node.module listed above and CVS as of yesterday, when an authenticated user is given view and revert permissions for stories, I get the following error when attempting to review the revisions tab for a story created by that user or by another user: user error: Unknown column 'grant_edit' in 'where clause' query: SELECT COUNT(*) FROM node_access WHERE (nid = 0 OR nid = 2) AND CONCAT(realm, gid) IN ('all0') AND grant_edit >= 1 in /home/cyber/public_html/drupal-cvs/includes/database.mysql.inc on line 99. When I try to set one as active, I get warning: Cannot modify header information - headers already sent by (output started at /home/cyber/public_html/drupal-cvs/includes/common.inc:460) in /home/cyber/public_html/drupal-cvs/includes/common.inc on line 292. And the Drupal page display is doubled with that same error. one page with "Access denied" and the other with "Page not found." ------------------------------------------------------------------------ Wed, 07 Sep 2005 07:06:15 +0000 : bonobo I also get some similar errors. I am using cvs from yesterday and the latest node.module (node_3, I believe) from this thread. Here is my set up: three users 1. first user = site admin 2. authenticated user -- has view/revert permissions on all node types 3. contrib user (in created role "contrib") -- has administer node permission, no view/revert privileges The site admin and contrib user can use view/edit/revert between various revisions with no problems. When user 2 (authenticated user) attempts to view a post that has been revised, the following error occurs: user error: Unknown column 'grant_edit' in 'where clause' query: SELECT COUNT(*) FROM node_access WHERE (nid = 0 OR nid = 6) AND CONCAT(realm, gid) IN ('all0') AND grant_edit >= 1 in C:\apachefriends\xampp\htdocs\drupalcvs\includes\database.mysql.inc on line 99. When the same user attempts to revert to an earlier version, the same error occurs. ------------------------------------------------------------------------ Wed, 07 Sep 2005 07:09:39 +0000 : bonobo And, I also get the same doubled display as Charlie -- Access denied on top, Page not found on bottom.

1 0

Returned Mail: [drupal-devel] [feature] Block query consolidation
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum. From: m3avrck <drupal-devel(a)drupal.org> To: drupal-devel(a)drupal.org Subject: [drupal-devel] [feature] Block query consolidation **************************************** Issue status update for http://drupal.org/node/30801 Post a follow up: http://drupal.org/project/comments/add/30801 Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: critical Assigned to: Anonymous Reported by: Allie Micka Updated by: m3avrck Status: patch (ready to be committed) Just chatted with mathias on IRC. I think there was some slight confusion and I apologize for this. The other issue is related to this issue indeed as it affects the block query and that is why Dries above mentioned to incorporate that fix into this one. Then both of these were rolled into the latest HEAD. However, a small oversight on my part, the key wasn't unique in all situations, and this above patch rectifies this situation. Now, in terms of performance, the key doesn't affect performance in any measurable way. Rather, it adds data integrity and structure to the table, and enhances other *unknown and future* queries. I chatted with a knowledgeable fellow on #mysql a while about this table and it was determined that a key *was* beneficial, even if any performance gains were not. In hindsight, maybe these patches shouldn't have been put together, but as such, they were, and this above patches fixes this issue and brings us back up to speed. So basically this patch "consolidates block queries *and* adds data integrity to the block table". :) m3avrck Previous comments: ------------------------------------------------------------------------ Fri, 09 Sep 2005 20:12:03 +0000 : Allie Micka Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB) The blocks module is running a separate query for each region, e.g.: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='left' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='right' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='header' ORDER BY weight, module SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND region='footer' ORDER BY weight, module Ultimately, we're going to need all of these so let's save a few queries. I have changed the query to: SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER BY region, weight, module And set all results to the static $blocks array on the first pass. ------------------------------------------------------------------------ Sun, 11 Sep 2005 23:50:53 +0000 : m3avrck Attachment: http://drupal.org/files/issues/block.module_5.patch (1.27 KB) Awesome patch! Noticed the version was set wrong so changed this to CVS. Also, rerolled the patch, there was an extraneous $region in there no longer needed. Reducing 4 queries is definetly a great start in optimizing Drupal :) Take this patch along with this one (might need to be updated now with this query rewrite I'm thinking): http://drupal.org/node/27157 ... and we'll have Drupal's loading of blocks pretty much maxed out with optimizations, then on to the slooow alias ones... ;) ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:24:56 +0000 : Souvent22 Gotta love when something is just logical. Makes sense, and the patch worked. I'd say this is ready. Get these blocks sped up a bit. This coupled with 'block primary key' patch should make the blocks solid. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:00 +0000 : Dries Has this been benchmarked? I would expect this to be faster, but it's good practice nonetheless. Feel free to throw that other block query patch into the mix. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:49:15 +0000 : m3avrck Dries, I ran some simple benchmarks with the devel.module. Queries with a default install were reduced from 125 to 121. Query times averaged around 50ms and this was reduced to roughly 47-48ms. It's not a *huge* savings, but on a a dev machine, with only one user, this is still good nonetheless. I imagine on a hard hit Drupal site this would start to scale up nicely with time/load savings. I'll reroll a patch that combines the two in a few. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:26:08 +0000 : Dries This patch reduces the SQL overhead but adds some CPU-overhead and memory-overhead. Hence, it's better to benchmark the number of requests/second (for example using ab or ab2) ... ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:35:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_14.patch (3.31 KB) Ok here is an updated patch, along with the blocks table modification of adding a PRIMARY KEY(module,delta). Spent sometime on #mysql discussing best practice for this table and adding a primary key is the best bet. Any indexes would not improve performance since this table will probably almost never have more than 30 rows in it. The primary key itself is slightly neglible as well, however, if any query were to perform advanced queries on this table, the primary key would benefit, so in best practice, we should have this as well. I'll try to get some benchmarks up soon backing these simple changes. ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:34 +0000 : Allie Micka This patch will not increase memory overhead unless a theme has one or more regions that are defined but not in use. Otherwise, the static $blocks array will be identical with or without the patch applied. The patch also does not increase CPU overhead because less code is processed. Without the patch, the entire query and parsing routine is executed 5 or more times (everything between if (!isset($blocks[$region])) { } ). With the patch, the same code executes only once. But the proof is in the pudding. I'm not sure what your standards are for ab, but I like to do 1 serial check and 1 check with concurrency so I can rule out performance gained by increased memory consumption, which bogs down concurrent requests. This patch appears to save an average of 9-12ms per request and increase performance by .05 requests per second. The relevant bits from my ab results are as follows: Before Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.32 [#/sec] (mean) Time per request: 430.20 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.16 [#/sec] (mean) Time per request: 4631.00 [ms] (mean) Time per request: 463.10 [ms] (mean, across all concurrent requests) After Patching ab -n 100 -c 1 http://localhost/ Requests per second: 2.37 [#/sec] (mean) Time per request: 421.08 [ms] (mean) ab -n 100 -c 10 http://localhost/ Requests per second: 2.22 [#/sec] (mean) Time per request: 4510.90 [ms] (mean) Time per request: 451.09 [ms] (mean, across all concurrent requests) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:04:27 +0000 : m3avrck Exactly running similar to what I was uncovering as well. So seems this patch is ready to go, time to commit :) ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:27:32 +0000 : Dries Thanks for the measurements! Patch committed to HEAD. Thanks. ------------------------------------------------------------------------ Tue, 13 Sep 2005 07:37:26 +0000 : asimmonds With this patch committed, and updating my HEAD install with update.php, I get the error "Duplicate entry 'user-0' for key 1" from mysql. I have two themes configured and there is a primary key (module,delta) clash between the two themes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:39:43 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_3.patch (1.96 KB) Eeek!!! Forgot to test with multiple themes, this patches fixes the key definition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:41:46 +0000 : m3avrck Attachment: http://drupal.org/files/issues/database_4.patch (1.96 KB) Noticed a typo in the PGSQL. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:00:19 +0000 : mathias I can recreate this bug when running update.php but I question the necessity of this index in the first place. I couldn't find one SELECT query in block.module that benefit from it. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:18:07 +0000 : Allie Micka For what it's worth, the benchmarks I submitted did not include the key/index patch, they only included the query consolidation patch. The performance impacts of the patch that hijacked this issue are unknown. I'd suggest moving this issue back to http://drupal.org/node/27157 , where you can assess performance impacts and the back out or correct that patch based on the results. Alternatively, continue using this issue but supply a few benchmarks to justify adding and/or removing the block table indexes. Thanks!

1 0

[drupal-devel] Release critical bugs for September 15, 2005
by drupal-devel＠drupal.org 15 Sep '05

15 Sep '05

/ bug reports email spam filtering does not "see" email addresses when they are NOT preceded by a word age: 34 weeks 5 days url: http://drupal.org/node/15654 / bug reports Duplicate entries error when changing themes age: 1 day 4 hours url: http://drupal.org/node/31223 Forum Topics do not show on forum overview pages. age: 2 days 14 hours url: http://drupal.org/node/31058 enable cache = blank page of death in current head age: 2 days 20 hours url: http://drupal.org/node/31031 Comment Pager does not render once threshold met age: 4 days 16 hours url: http://drupal.org/node/30889 Delete filter format fails age: 6 days 3 hours url: http://drupal.org/node/30781 can't submit comments age: 6 days 16 hours url: http://drupal.org/node/30743 logout fails age: 1 week 10 hours url: http://drupal.org/node/30676 Submit button needs double-press when menu options dropped-down age: 2 weeks 1 day url: http://drupal.org/node/30097 update.php is broken when drupal is installed in subdirectory age: 2 weeks 1 day url: http://drupal.org/node/30075 Drupal "forgets" some blocks... age: 2 weeks 6 days url: http://drupal.org/node/29733 system_settings_save() bypasses access_control age: 2 weeks 6 days url: http://drupal.org/node/29680 files table needs a "module" column age: 3 weeks 4 days url: http://drupal.org/node/29297 Dont't work rewrite for apache 2 assigned: Алексей age: 3 weeks 5 days url: http://drupal.org/node/29238 logo uploads still broken...? can't replace default logo with new drupal 4.6.3 tar ball? age: 3 weeks 5 days url: http://drupal.org/node/29221 Error when anonymous user creates content age: 4 weeks 1 day url: http://drupal.org/node/29032 Reset user mail variables. age: 4 weeks 3 days url: http://drupal.org/node/28868 Free Tagging broken on forums age: 5 weeks 1 day url: http://drupal.org/node/28607 js addLoadEvent not working. age: 6 weeks 4 days url: http://drupal.org/node/27884 Forum bug with drupal 462 age: 7 weeks 10 hours url: http://drupal.org/node/27663 enable MySQL client side for UTF8 age: 8 weeks 2 days url: http://drupal.org/node/26990 Drupal 4.6.2 Doesn't allow login (IIS 5.1, PHP 5.0.4) assigned: bigeye age: 8 weeks 6 days url: http://drupal.org/node/26780 Comment pager not working in cvs? age: 11 weeks 20 hours url: http://drupal.org/node/26031 postgresql autocomplete code age: 11 weeks 21 hours url: http://drupal.org/node/26027 Accidentally Deleting Forums Vocabulary in Categories Breaks Forums? age: 14 weeks 5 days url: http://drupal.org/node/24274 Taxonomy Multiple Hierarchy doesn't display children for all its parent age: 15 weeks 1 day url: http://drupal.org/node/24023 Getting term node counts fails without "Administer Nodes" permission age: 15 weeks 2 days url: http://drupal.org/node/24015 Mysql Password with special symbols assigned: zignit age: 19 weeks 4 days url: http://drupal.org/node/21719 4.6 aggregator showing/not interpreting HTML tags age: 23 weeks 5 days url: http://drupal.org/node/19874 User deleted - nodes still "crippled", NOT deleted! age: 25 weeks 6 days url: http://drupal.org/node/19098 New submit error, non-admin user, postgresql database assigned: adrian age: 27 weeks 2 days url: http://drupal.org/node/18552 user redirected to homepage when logging in age: 27 weeks 6 days url: http://drupal.org/node/18381 Change to sesssion.inc violates UID database constraint age: 34 weeks 4 days url: http://drupal.org/node/15666 forum.module problem with postgresql v7.4.1 age: 35 weeks 3 days url: http://drupal.org/node/15411 PHP 5 gives error when session table has no session assigned: adschar age: 35 weeks 3 days url: http://drupal.org/node/15399 Argument NOT must be type boolean age: 43 weeks 3 days url: http://drupal.org/node/12950 Comments in approval queue still show up in tracker age: 47 weeks 5 days url: http://drupal.org/node/11647 Twin problems with comments assigned: Junyor age: 49 weeks 5 hours url: http://drupal.org/node/11366 user error: Duplicate entry assigned: Kjartan age: 1 year 40 weeks url: http://drupal.org/node/4428 / feature requests Site Slogan Margin age: 5 weeks 4 days url: http://drupal.org/node/28382 Filter the title to allow HTML comments age: 8 weeks 4 hours url: http://drupal.org/node/27205 moving pages en masse age: 24 weeks 5 hours url: http://drupal.org/node/19754 Taxonomy module should use nodeapi 'load' age: 27 weeks 1 day url: http://drupal.org/node/18631 Auto PHP memory maximisation age: 29 weeks 3 days url: http://drupal.org/node/17663 Allow named anchors to work without specifying full path to node age: 1 year 42 weeks url: http://drupal.org/node/4213 / support requests blocks age: 12 weeks 3 days url: http://drupal.org/node/25378 Problem with Forums age: 35 weeks 5 days url: http://drupal.org/node/15299 / bug reports search over fields.inc fields conceptual error age: 45 weeks 6 days url: http://drupal.org/node/12259 fields.inc screws up with multible multiselect fields age: 45 weeks 6 days url: http://drupal.org/node/12257 / bug reports Menu item not showing age: 49 weeks 6 days url: http://drupal.org/node/11210 / bug reports segmentation fault related to filestore age: 1 year 51 weeks url: http://drupal.org/node/3001 / bug reports file size bug age: 13 weeks 6 days url: http://drupal.org/node/24728 / bug reports gallery2 embeded bug with RC1 age: 4 weeks 20 hours url: http://drupal.org/node/29090 gallery bug - not integrated in drupal assigned: vidy age: 5 weeks 1 day url: http://drupal.org/node/28598 gallery.module doesn't synch passwords when they're changed age: 7 weeks 2 hours url: http://drupal.org/node/27702 Full screen slideshow in Gallery module is not working (erroring out on the wrong URL) age: 10 weeks 21 hours url: http://drupal.org/node/26479 Drupal session being logged out each time gallery module is called age: 12 weeks 4 hours url: http://drupal.org/node/25614 Gallery error age: 20 weeks 3 days url: http://drupal.org/node/21290 Fatal Error: variable_get() age: 25 weeks 4 days url: http://drupal.org/node/19189 No Gallery Image Block age: 26 weeks 9 hours url: http://drupal.org/node/19045 Mini applet can't find gallery at url / age: 26 weeks 5 days url: http://drupal.org/node/18785 Fatal error: Undefined class name 'galleryembed' age: 29 weeks 2 days url: http://drupal.org/node/17746 / support requests Can't Embed Gallery into Drupal age: 28 weeks 4 days url: http://drupal.org/node/18029 / bug reports Drupal-4.4-rc breaks registration setups and groups.module age: 1 year 26 weeks url: http://drupal.org/node/6392 / bug reports Path for image folder not inserted correctly when nodetype = image age: 2 weeks 2 days url: http://drupal.org/node/30031 image module patch & image_import module age: 3 weeks 3 days url: http://drupal.org/node/29377 image module fails on dual site configuration age: 9 weeks 17 hours url: http://drupal.org/node/26657 Editing a previously uploaded and thumnailed image loses the thumbnail. age: 12 weeks 2 days url: http://drupal.org/node/25465 Cannot upload .jpgs age: 28 weeks 3 days url: http://drupal.org/node/18076 Image.Module does not know the correct filename age: 43 weeks 2 days url: http://drupal.org/node/13032 / feature requests Image deletions? age: 1 week 1 day url: http://drupal.org/node/30557 / support requests Images are there but not showing assigned: Stepfordtwit age: 7 weeks 1 day url: http://drupal.org/node/27615 Image Module Permissions age: 10 weeks 4 days url: http://drupal.org/node/26280 path problems on upgrading... age: 19 weeks 4 days url: http://drupal.org/node/21725 / bug reports RSS "no element found at line 1" age: 10 weeks 6 days url: http://drupal.org/node/26185 Outdated help texts age: 1 year 6 weeks url: http://drupal.org/node/9692 Import doesn't like XML/RSS feeds with no Title element age: 1 year 15 weeks url: http://drupal.org/node/8128 / tasks move XML-parser into an inclde file. age: 12 weeks 2 days url: http://drupal.org/node/25439 / feature requests Notify.module ignores the true submission queue age: 1 year 46 weeks url: http://drupal.org/node/3765 / feature requests Internationalization support for PDF assigned: TheLibrarian age: 1 year 24 weeks url: http://drupal.org/node/6795 / bug reports Reminder email links don't work age: 11 weeks 6 days url: http://drupal.org/node/25677 / bug reports access control problems age: 7 weeks 5 days url: http://drupal.org/node/27273 Projects not listed and page not complete age: 15 weeks 2 days url: http://drupal.org/node/23956 Date for latest is incorrect age: 22 weeks 12 hours url: http://drupal.org/node/20485 Invalid link to release files age: 29 weeks 4 days url: http://drupal.org/node/17625 Node body not complete w/o releases age: 31 weeks 6 days url: http://drupal.org/node/16733 blank page after install age: 32 weeks 1 day url: http://drupal.org/node/16575 / support requests Help with everything - nothing seems to work in the latest build age: 42 weeks 3 days url: http://drupal.org/node/13310 / bug reports Error with URL to recipe age: 2 weeks 3 days url: http://drupal.org/node/29885 / bug reports Can't get the inline tag to show series age: 18 weeks 2 days url: http://drupal.org/node/22520 / feature requests Request to generate email for all content changes/additions, including comments age: 8 weeks 5 days url: http://drupal.org/node/26824 / bug reports javascrip downloads square.gif for every category age: 30 weeks 2 days url: http://drupal.org/node/17366 / bug reports Sends trackbacks for unpublished nodes assigned: ankur age: 33 weeks 1 day url: http://drupal.org/node/16239

1 0

[drupal-devel] [feature] Redesign permissions page
by fajerstarter 15 Sep '05

15 Sep '05

Issue status update for http://drupal.org/node/30843 Post a follow up: http://drupal.org/project/comments/add/30843 Project: Drupal Version: cvs Component: user.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: fajerstarter Updated by: fajerstarter -Status: active +Status: patch (code needs work) Maybe it's a bit too bold to call this handcoded example [1] a patch, but it can be made to one... Anyway, I've been playing with some javascript on the permissions page. I used some of the ideas in my mock-ups. Even though I like the side-tabs in the first mock-up I went for the already established form group approach. Some things are missing in the javascript (like auto-collapse) but that should be easily fixed. The css could be cleaned up as well. My biggest problem so far is how to "inject" the tbody tags that are used to group the table rows (they are hancoded in my example). Currently the table theme function doesn't allow that. Someone want's to give it a go? The role toggle is plain javascript in this example. In order to have this functionality for non-javascript users it has to be a regular form. We could of course also just hide it for non javascript user. AJAX could also be considered. [1] http://www.student.lu.se/~hek02afa/sandbox/access/adminaccess.html fajerstarter Previous comments: ------------------------------------------------------------------------ Sat, 10 Sep 2005 10:16:25 +0000 : fajerstarter Attachment: http://drupal.org/files/issues/permissions_UI.gif (18.57 KB) Here's a mock-up of a javascript enhanced permissions page. Just wanted to share the idea with you; it's just an mock-up, no code yet. The idea is to give users with javascript the possibility to filter the permissions. Users without javascript gets the same page as before. It's similair to the collapsible form groups approach used in the upcoming Drupal 4.7. This article [2] is an example how it could be coded (unobtrusively). Thoughts? [2] http://www.bobbyvandersluis.com/articles/unobtrusiveshowhide.php ------------------------------------------------------------------------ Sat, 10 Sep 2005 12:51:58 +0000 : Bèr Kessels I really like the idea, but with a few comments: We should not start introducing side-tabs. Ratheruse tabs across the top, in some manner. That is consistent. The JAvascript must off course degrade well. In fact, it should by default not use any JS, just show links to pages. If it is proven that the browser supports it, js can be switched on (automatically, off course). ------------------------------------------------------------------------ Sat, 10 Sep 2005 13:25:46 +0000 : fajerstarter Ofcourse it should degrade well, that's why I pointed to the article and also referenced to the collabsible form group already in core. The term unobtrusive is often used for that kind of behaviour. ------------------------------------------------------------------------ Sat, 10 Sep 2005 14:19:29 +0000 : timcn Good idea. We could use collapsible fieldsets as already used in the cvs head. ------------------------------------------------------------------------ Sat, 10 Sep 2005 14:34:56 +0000 : Allie Micka In a way, I kind of like the side buttons. They might look weird and cluttered next to a left navigation menu - especially in a fixed-width theme - but it "scales" better. Tabs along the top are going to be problematic. After a certain number of modules, you're pushing things out way too wide. A vertical arrangement does not suffer the same problems, which is why I wouldn't reject it outright. My biggest problem with the access page now is its height. After scrolling down to see options, I tend to forget which column belongs to which role. Also, when we add a number of roles, the screen gets way too wide. Breaking them up by module is useful, and perhaps this can lead to folding the roles so they don't "push out" too much. Great Start! ------------------------------------------------------------------------ Sat, 10 Sep 2005 16:10:49 +0000 : killes(a)www.drop.org Adding another column to a matrix that is already very wide if you have more than just the basic roles doesn't really seem to be a good idea. ------------------------------------------------------------------------ Sat, 10 Sep 2005 21:05:36 +0000 : Bèr Kessels I agree. IMNSHO we should get rid of that matrix. OR better: leave the matrix there for thos who thing they need it, but come upp with a new idea/concept. The matrix does not scale, we all know that. And I am not talking about five modules and thre roles. But about fifteen roles and twleve modules with /loads/ of permuissions. Let us please first think about this a little more. (mockups are great for this, hint) ------------------------------------------------------------------------ Sat, 10 Sep 2005 21:21:34 +0000 : kbahey Just to point out the obvious, the matrix makes it easy to compare permissions for roles. It also allows bulk editing of permissions for multiple roles at a glance. We will lose these when we junk it. It does not scale though. As far as keeping two interfaces, it may end up being confusing, and add to code bloat. ------------------------------------------------------------------------ Sun, 11 Sep 2005 08:48:48 +0000 : fajerstarter Attachment: http://drupal.org/files/issues/access_UI_select_roles.png (4.5 KB) This mock-up adresses the left too right scaling issue. For non javascript users we provide a filter button, so the filtering can be done with regular POST. It would be usefull if the role filtering was accomplished with AJAX for javasript users (no page reload when a role is disabled or enabled in the matrix). I still feel that the side-tabs is best suited to adress the scaling issue with a lot of modules (and I kind of like it :) ). But I guess the collabsible form group approach could be used instead. ------------------------------------------------------------------------ Sun, 11 Sep 2005 21:14:19 +0000 : fajerstarter Just some realated issues to take in account: http://drupal.org/node/28301 http://drupal.org/node/25530 (especially comment #9) And some background/motivation http://drupal.org/node/30711 ------------------------------------------------------------------------ Mon, 12 Sep 2005 19:59:42 +0000 : Gabriel Radic fajerstarter: The javascript permission groups by module are not really my cup of tea. I personally *like* having all the permissions visible at once. However, I find your group checkboxes mockup really great. Now, if *that* was dynamic, as in show/hide columns with JS, it would roxr. Just my ¢2.

1 0

[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by m3avrck 15 Sep '05

15 Sep '05

Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck Status: patch (ready to be committed) Ready to go! m3avrck Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:06:44 +0000 : Junyor FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB) Updated JS styling and JS-killswitch after talking with Unconed on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck Just to build on Junyor's comment [6] this functionality is built into Opera 8 and it *doesn't* produce any errors. [6] http://drupal.org/#comment-44066 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB) Updated patch to fix possible problem of overwriting onsubmit event handler thanks to Thox on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:13:59 +0000 : webchick I tested this and can confirm that it works in *both* versions of Safari that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So looks like this code goes one better over the Blogger.com stuff, because their stuff doesn't workk in 1.3. Nice job! ;) ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck Well I'm gonna set this to commit then. Tested and working on IE, FF, Safari. Doesn't work or break Opera or Konqueror but not needed in these cases. Thox and Unconded have offered thoughts and I've modified code as needed. Doesn't seem like there is anything else left except a commit :) ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:13:48 +0000 : Bèr Kessels In general I dislike the feature. Konqueror somehow dies this, and that is the way it /should/ be. Browsers should take care of this, not the web app. But, since it is only konq. this patch has a good enough additional value :) I would like to see this patch tested with, tinyMCE and HTMLAREA, at least. For I am quite sure this will break these modules so bad that they are near unusable. Which brings me to the next point: using the textarea hook a simple module could take care of this. I would very very much prefer this living in a contrib, or even a core module. Just not "enforced" on me. And the last point: if this is for core, please use that textarea hook too. This is where that hook is for: extending the textareas. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:32:25 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_18.patch (2.23 KB) Updated patch after talking with Thox and Uncloned to attach this event only to forms with a specific class (hence avoiding the problem of being on an edit page with other admin/search forms). ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:32:55 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_4.js (1.67 KB) Updated JS file. ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:45:21 +0000 : m3avrck Berkes, this patch *does not* break TinyMCE, just tried. However, TinyMCE does not take into account this script. This should be an easy patch to TinyMCE. Also, this doesn't affect just textareas, it affects all fields within a given form (e.g., text fields, check boxes, etc...). Sure the blunt of editing/creating a node is in the textarea, but there are still all of those other changes that can be made (new title, revision status, front page promotion, etc...) so this needs to account for them all which it does. This script is unobtrusive and degrades perfectly well. It is tested and working in FF, IE, Safari and doens't cause problems in Opera or Konqueror which don't need this feature anyways (since they already have it). The usability boost of this script is too enormous *not* to include in core. As a contributed module, it is really too flaky, and along those lines, autocomplete.js, inlineuploads.js and related should be in their own contributed modules :) I do see your points and I hope this clears it up. It doesn't cause any problems and only attaches to specified forms, and is off by default. Any module can easily make use of this script when they use: form(..., TRUE). This is the same behavior as the other JS files included with HEAD as well. Once this is in core I'll work on a patch to TinyMCE to get that up to speed ;-) (and as such, TinyMCE still works fine, no probs/errors/etc, and good reason too if you look at how the code *actually* works ;-)). ------------------------------------------------------------------------ Tue, 13 Sep 2005 17:54:37 +0000 : Bèr Kessels I see. the "why" for not using form textarea is perfectly valid. And I tried to explain that, eventough I feel this belongs in the Agent, it is a usability enhancement for all the others using sillier browsers ;). And you are right about that part of contributions, exp since you cannot use hook_texarea, having this in a contrib cannot be achieved. I hereby retract my hesitations. (though I have no time to reapply the latest patch and test it on konq. The last JS updates broke drupal on konq, which Steven fixed, right away though!)

1 0