September 2005 - Development - lists.drupal.org

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] [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

[drupal-devel] [task] Adding taxonomy terms has no feedback
by m3avrck 15 Sep '05

15 Sep '05

Issue status update for http://drupal.org/node/31151 Post a follow up: http://drupal.org/project/comments/add/31151 Project: Drupal Version: cvs Component: taxonomy.module Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: m3avrck -Status: patch (code needs work) +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/taxonomy.module_10.patch (957 bytes) New patch that fixes the goto() and sends you back to add another term, displaying a message that a term has been added. Tested with free tagging, this patch does not output any messages saying "term 1 created", "term 2 created" , etc. All that is output is "Page was created" so Morbus your issued is already addressed elsewhere. m3avrck Previous comments: ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:08:31 +0000 : drumm Adding a taxonomy term should trigger a drupal_set_message() so people know somehting happened. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:25:27 +0000 : Souvent22 Drumm, I have it fixed/patched, but before I post, we got some options: Current way is that after adding a term, it stays on the "add terms" page.... So... A) After adding, take me back to add terms for another one. B) After adding, take me back to my list of terms for that vocab. C) Give me a check box that says "Add another after sumbission" During SAT's, they always said B was the right answer...or was it C..hmm....ideas suggestions? ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:31:40 +0000 : m3avrck I would say go with B. That is the general behavior in the rest of Drupal, might as well be consistent. And the checkbox doesn't help that much... extra click (I'm assumed off by default) and the user can just click 'add' again anywho. ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:32:42 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/terms.patch (943 bytes) B it is. ------------------------------------------------------------------------ Thu, 15 Sep 2005 14:07:19 +0000 : m3avrck Attachment: http://drupal.org/files/issues/terms_0.patch (943 bytes) Tested patch, definetly improves usability of taxonomy module and adding terms. Before, it wasn't clear if a term was added or not. This also makes adding a term more consistent with how the rest of Drupal works (e.g., taking you back to the main add term page instead of a blank form wondering what just happened). Fixed coding style of patch. Ready to go. ------------------------------------------------------------------------ Thu, 15 Sep 2005 14:12:34 +0000 : Morbus Iff Somewhere along the line, I think "visual feedback when terms were added" was actually REMOVED when the free tagging patch hit core. Consider: you write a node, with free tagging enabled, and you create 10 new terms. Upon successful submission of a node, you get ten messages saying new terms were added. That may be find for an administrator, but I certainly wouldn't want a regular non-admin or non-taxonomy-admin user seeing a message that they've added a new term. ------------------------------------------------------------------------ Thu, 15 Sep 2005 14:13:03 +0000 : robertDouglass +1 for the message. I thought that going back to the add-term form was by design, and personally, I like it. I often have to add lots of terms in a row, to the same vocab, and this saves me a page load each time. So -1 for drupal_goto('admin/taxonomy/' . arg(2));. ------------------------------------------------------------------------ Thu, 15 Sep 2005 14:14:39 +0000 : robertDouglass RE Morbus's comment - we could check to see if the user is looking at the admin/taxonomy/vid/add/term path and only show the message then. ------------------------------------------------------------------------ Thu, 15 Sep 2005 14:15:13 +0000 : Morbus Iff (Toggling back to "needs work" per my comment in #5 - Rob's comment and my own crossed in the stream). ------------------------------------------------------------------------ Thu, 15 Sep 2005 14:33:23 +0000 : Kobus I am with Robert here (#6) +1 for the message, -1 for the drupal_goto. The feature of adding several terms after each other is very handy. Kobus

1 0

[drupal-devel] [feature] Separate mail backend
by killes 15 Sep '05

15 Sep '05

Issue status update for http://drupal.org/node/28604 Post a follow up: http://drupal.org/project/comments/add/28604 -Project: Simplenews +Project: Drupal -Version: cvs +Version: cvs -Component: Code +Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Dries Updated by: killes(a)www.drop.org Status: patch (code needs review) Note: I am moving this to the main project. VwX: What I see looks very nice for a start. Some polishing is certainly needed, but I'd like to try this. If I only had the time... killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ Wed, 10 Aug 2005 12:35:37 +0000 : Dries It was suggested that we separate the mail backend from the front-end. We might want to include the mail backend in core as includes/mail.inc. Then, other modules like massmailer, subscriptions, notify, pm (private messages), project (project issues), contact, etc. could reuse this component. The mail.inc file would implement some kind of mail queue functionality, and modules just add a mail to the mail queue using a simple /mail API/. In future, the mail backend could deal with bounces and report back proper status codes. Moreover, mail.inc would be pluggable so it could be replaced by a more powerful one, or one that is build specifically for the underlying mail transport (SMTP server). Furthermore, this would solve issues with how many mails get send within a specified interval. Like, when more than one module sends out e-mails it is hard to enforce limitations/restrictions imposed by the hosting company. I'm posting this here so you can keep this in mind when working on simplenews, and to solicit for feedback. Chances are we'll setup an IRC meeting to discuss this further. Details to follow. ------------------------------------------------------------------------ Wed, 10 Aug 2005 16:15:16 +0000 : Amazon A good place to start would be to look at existing Mail API's Pear Mail: http://pear.php.net/package/Mail Pear Mail tutorial: http://www.zend.com/pear/tutorials/Mail.php#Heading2 ------------------------------------------------------------------------ Wed, 10 Aug 2005 20:21:57 +0000 : walkah um. +1 and then some. I think this is good... and some good references from Amazon. I'll throw in one other : http://phpmailer.sf.net/ - has great mime handling (i've run into issues with PEAR::Mail's mime handilng in the past, but that was around 2 years ago, and things may have gotten better since then). Oh - and you don't mention it - but support for multipart / mime messages would be lovely :) I'd be interested in / happy to sit in on any IRC discussion as well. ------------------------------------------------------------------------ Wed, 10 Aug 2005 21:40:34 +0000 : robertDouglass phpmailer++ I've used it and like how easy it is to send both text and html mail. Did I say html mail? You bet! It's time that we had the option. ------------------------------------------------------------------------ Thu, 11 Aug 2005 05:47:01 +0000 : cyberchucktx I'm definitely interested. Hopefully by adding a post to this I'll get notified on new postings (?) If not I may miss the IRC .. I've been doing a lot of work with safe_mode PHPLIST (have posted to the drupal site somewhee, can dig out the reference if anyone is interested). Charlie in TX ------------------------------------------------------------------------ Wed, 17 Aug 2005 08:42:34 +0000 : Dries It was suggested that mail.inc also provides a HTML to text services. Lots of modules (newsletter, notify, project) try converting HTML to text. Having a reusable function makes sense, and will lead to better conversion routines. XSLT anyone? ------------------------------------------------------------------------ Wed, 17 Aug 2005 12:31:27 +0000 : Grugnog2 +1 for phpmailer I found it very easy to use - and most importantly for me - it supports SMTP authentication. As many ISP's will not let you send mail without using SMTP-auth nowadays it may be worth this feature is incorporated into the mail.inc API. - Grug ------------------------------------------------------------------------ Wed, 17 Aug 2005 15:08:43 +0000 : Robert Castelo For about a year now I've been working on an email newsletter and announcement module [1]. There's a version in my sandbox commited about 2 months ago which works fairly well. Dan Robinson and Varr Willis at CivicActions, Moshe Weitzman plus a few others have been testing it, and I've had lots of positive feedback and feature and bug reports from them. A few months ago a realised that there's not much point in putting so much effort into creating all this functionality, only for it to be locked away in my module. Better to Better to create discreet component modules which provide a particular service, such as bounced email handling, but which can be used by any other module that also needs that service. For the last two months I've been working to split functionality into component services modules and make these services available to all modules. One of the biggest challenges was to make these component modules independent of each other. The only area where I haven't managed this is some of the database calls - but thanks to a chat with chx I realised that db_rewrite_sql could be used to handle this very nicely. This is what I have: * bounced_email.module - process bounced emails * html2text.module - convert HTML to plain text equivalent (e.g. list item becomes "* item") * identity_hash.module - manages full and partial loggin based on hash which can be used in email links * publication.module - defines and packages content of publication, which could be any kind of publication * schedule.module - defines and manages schedules, e.g. email sending schedule * subscribed.module - manages subscriptions to publications * templates.module - manage and define templates What's great is that the component modules are not limited to email, they could be used to quickly create RSS newsletters, PDF newsletters, text message newsletters, or even personalised/filtered website sections. I haven't made the new component modules available anywhere yet, but I'll be happy to upload them to contrib and let others get involved. What would be the best way to commit them - as a single directory? or each component module in it's own directory? [1] http://www.cortextcommunications.com/development/newsletter/features ------------------------------------------------------------------------ Thu, 15 Sep 2005 02:05:18 +0000 : vwX Attachment: http://drupal.org/files/issues/mailqueue.tar.gz (3.71 KB) This is a very basic mail queue system I would like to contribute. There are some changes to other modules that will have to be made such as changing user.module to use this instead of directly calling the php mail function and moving mime_header_encoding to mail.inc.

1 0

[drupal-devel] [feature] add inline functionaliy to uploads
by m3avrck 15 Sep '05

15 Sep '05

Issue status update for http://drupal.org/node/26288 Post a follow up: http://drupal.org/project/comments/add/26288 Project: Drupal Version: cvs Component: upload.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Bèr Kessels Updated by: m3avrck Status: patch (ready to be committed) Patch in #60 does work as advertised. The noted bug above is indeed a seperate and indepedent issue and can be recreated without this patch. In otherwords, this patch doesn't introduce any bugs and is ready to go. m3avrck Previous comments: ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:03:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline.patch (19.05 KB) One of the most often asked features is proper inlnie handling of files. Look at the amount of solutions, the popularity of image_assist, and the amount of peolpe dowloading image.module! That alone should be enough proof that Drupal lacks proper inline image support. This patch adds that to core. In fact, it does little more then appending a link of img tag to the body or the teaser. Off course that is configurable per file. Next to the [] list checkbox, this patch adds an [] inline checkbox. Simplicity is the foundation of this patch. I want no stles for inline editing, no fancy html wrappers, no tokens, just $node->body or teaser appended with a small html string. Another small themable funtion is introduced, (hey, you cannot expect me to develop something without adding more power for themers, now, can you? ;) ), that allows people to theme the string that is appended to the body or the teaser. Oh, and also note hat the biggest part of this patch is some cleaning I had to do in order to be able to develop properly. I dont like Ifs inside cases in foreaches inside swiches. in other words: nodeapi now calls functions instead of executing code directly. ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:19:25 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/inline.patch.screenshot.png (26.68 KB) here is how the form now looks ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:19:58 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/inline.patch.screenshot3.png (30.53 KB) and this is an example of inlined images and a .doc file. ------------------------------------------------------------------------ Sun, 03 Jul 2005 20:44:00 +0000 : sepeck changing to patch per request from berkes ------------------------------------------------------------------------ Tue, 05 Jul 2005 07:46:13 +0000 : Kobus This gets a +1 from me in principle, however, the [inline:xx] tag with inline.module gives you greater freedom as to where the inline image must be displayed. If you can add this functionality (I haven't checked the code, I don't know if it is in there) it would be a great addition for Core. This same strategy can be used for inline blocks, I am sure. Regards, Kobus ------------------------------------------------------------------------ Tue, 05 Jul 2005 08:57:50 +0000 : Bèr Kessels there are a couple of reasons why i did not include the [inline] tags. * I aimed for extreme simplicity: a checkbox shows an image inline: its up to the theme where it appears (if one does not like it before/above the body and teaser.). Simplicity was the main goal. * We don't have any tokens in core. And we should not have them. * Tokens are a very bad substitute for a good interface. They give less power then plain HTLM. Are much worst documented then HTML, but in the mean time, they are still as hard to learn as HTML. (Yes, I know people _think_ they are easier, but there _is_ not difference between [ ] and , only that its a different ascii char. So, no. I don't allow any placement of the image. I leave that that for dedicated modules, or the themer to decide. ------------------------------------------------------------------------ Tue, 05 Jul 2005 09:34:06 +0000 : Kobus So you will provide an API to do this? For example, with perhaps minor modifications, the inline.module will be able to display these files? In that case I am happy. If not, then I can't give my support to this patch (as if that matters...). A themer can't do this task as inline images (and blocks for that matter) is too dynamic for theming; it can be placed anywhere in a node where the user pleases. This means for me that there should be a module for this, such as inline module that allows you to define [inline:xx] tags. If your module emits an array of uploaded files (such as upload.module), inline.module can be, with minor changes, adapted to show these files inline. To show you what I mean with the content is too dynamic for a themer to perform this task, look at this screenshot related to inline blocks (inline images can follow the same pattern): http://drupal.org/files/issues/regions---possibility-3.png ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:26:15 +0000 : Bèr Kessels Kobus, We are dealing with different issues here: You want a method to place images, files or so anywhere in the post. That is fine, but certainly NOT addressed in the patch! I made patch that *only* adds marked files to the body. its really nothing more then <?php $node->body = $filestring . $node->body ?> No APIS, no, dynamic tokens, no filters, nothing. However, what I meant with themers, is that there now is a theme_upload_inline available, so you can theme the abovementioned $filestring. On top of that $node->files[FILEID]->inline is TRUE if a file is flagged for inline. So in node.tpl.php, or wherever you want to theme a node, you can print nice images inline, when that flag is set. And about the comment that a themer cant do this: Simply not true. On most sites images are always placed in the same places. REally, even the sites see which use img_assist or inline, use them to place the images on the exact same places in every node. People /think/ they want the power to place images anywhere, but they hardly ever use that power. Just look at all the big news/publishing sites out there (BBC, CNN, BoigBoing, r even freshmeat) images are all placed acc. to the theme. They are not placed in random places by authors. So if you are one of the few that still want that power, there aer mots of power tools like inline module or img_assist. We should offer a good default, one that is simple. ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:41:34 +0000 : Gerhard Killesreiter I fully agree that tags are evil. Kobus: You can always look for tags in $node->body in your theme's node function. ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:50:52 +0000 : stefan nagtegaal Ber, the theme-function in your code looks like: <?php +/** + * Theme function for rendering of inline images + * @param $file a file object. + * @param $image a Boolean, indicating whether an img tag (TRUE) or an anchor tag (FALSE) should be used. + * @ingroup themable + */ +function theme_upload_inline($file, $image) { + if ($image) { + return '<img src="'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'" alt="'. check_plain($file->filename) .'" class="upload inline">'; + } + else { + return ''. check_plain($file->filename) .' [1]'; + } + +} ?> After looking at your code it's not clear to me when $image is TRUE or FALSE, can you elaborate on me please? [1] http://drupal.org/'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'\" class=\"upload inline ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:06:39 +0000 : Bèr Kessels The function calling the theme function should decide whether its an image. IMO that is far too hardcore code for a themer ;) <?php $image = ereg('^(image/)', $file->filemime); ?> inside _upload_inline() does the trick. I did find one issue, though, with svgs, which are image/svg so maybe we should limit this to really only inline jpg, png, and gif, by extension? But I am no fan of determining files by extension, and IMO getimgsize is too heavy; ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:13:52 +0000 : stefan nagtegaal Can't you make use of PHP's <?php image_type_to_mime_type(); ?> or <?php image_type_to_extension(); ?> ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:45:43 +0000 : Kobus Well, if not in the patch, I need inline functionality one way or another. inline.module works perfectly with the current upload.module to fulfill my needs, and if there is a way where someone can extend your proposed upload.module, that would get my +1, otherwise I will just remain neutral about this issue. inline.module makes use of $node->files and displays that image. If your patch still uses $node->files, then this is a non-issue. In other words, if I can, with inline.module, or with minor modifications to inline.module still show files inline, I am +1 for this. As for the themability question... I still disagree. You say "Simply not true. On most sites images are always placed in the same places." That is ONLY true because they have NO alternatives. Means the content is in the same places, because they have no other places to put them. I have made tens of static sites where the content does not resemble a fixed pattern or structure. For me, free-flow layout is my expression of my creativity. This is why I still even BOTHER with static sites; when Drupal don't do what I want it to do. If Drupal could do inline display of content properly, I'd never even build another static site. It is a great mistake people make (including myself), and that is they design the site before ANY content is available, and this happens a lot in static sites. But in dynamic sites, you have far less control over this, unless you have a VERY clean structure with limited information that can be added, and only a few people posting content. If you can have your content before you start the design, you will have a better fit. I can't see how you can anticipate every possible posting posted on your site if you have a diversity of users. Especially not if you're using a site such as an designer's showcase site where your creativity is shown by your web sites. Possible, but not easy. If I claim that I have "unique, fresh designs" I can certainly NOT give them a "box-like" site with "left and right side bars only". That's why I need inline content, this includes images AND boxes. Gerhard: Your concern about tags is answered as well above, if I am not mistaken. The patch need not provide the tags, but should provide for someone to develop a module that provide the filters or tags. To summarize: I don't need your patch to do the inline images. I just need your patch to be able to allow someone else to develop a module that can display inline files. Kobus ------------------------------------------------------------------------ Mon, 18 Jul 2005 09:56:38 +0000 : Bèr Kessels okay, so for all clarity, let me sumarise: * this patch introduces a chackbox "inline". If checked, the file will show up inline. IT will be appended to the teaser and the body. * this patch does *not* allow one to place images of files in a userdefined place in the body. * this patch does *not* do any resizing nor any thumbnailing. * This patch is meant to be simple, clear and transparant. No tokens, no javascript, no nothing. ------------------------------------------------------------------------ Mon, 18 Jul 2005 11:21:54 +0000 : Kobus Then I can't see how this patch could replace the existing upload.module, which allows you to use inline.module to place images inline. I therefore withdraw my +1 and go -1 on this (not that it matters, I believe). I think you didn't properly read my previous message. I said I don't need your patch to do all this, but I can't live without the functionality currently provided by the upload.module and inline.module combination. I agree that your patch don't need to have that functionality, but if your patch takes away this functionality, in other words, I can't (by hook or by crook) manage what I need to do, my vote is -1. Regards, Kobus ------------------------------------------------------------------------ Mon, 18 Jul 2005 19:58:34 +0000 : Bèr Kessels Kobus, And all the others looking fofr advanced inline systems: please do not -1 this. It will hep inline module a lot too. allthough not yet in this patch, but can you imagine a perission system for inline module, that comes for free? Or a core system, that makes inline module ten times smaller? it is this path! so, please, if it is not /exactly/ what you are looking for, at least look at the advantages for Joe Average, and even for the possibilties for your favorite inline-module. even imag_assist will gain an anomous usability impcact when usiong this module, for it can then finally use the upload module in full scale. Ber ------------------------------------------------------------------------ Sat, 23 Jul 2005 18:59:10 +0000 : Bèr Kessels Sohodjo jim: " I hope it matters. A big -1 on enforcing over-simplification at the expense of important and existing functionality. If automatic, uncontrollable placement of images becomes the standard, it will mean fewer not more image-rich Drupal sites as site owners/developers get complaints about "Why can't I control my images!" I _strongly_ encourage Ber to provide a "have it both ways" solution. Why not have an admin configuration setting for "allow inline tags" with default off. Change it to on and you get an additional upload checkbox for 'inline at tag' to accommodate the current functionality of the inline module while simultaneously allowing for default placement. " NOOO. people; please; look at the PATCH. Again: and hopefully last time: Simplicity. This patch does NOT replace ANYTHING inline module or img_assist wants to do. This patch hands better data to such modules. It hands a variable over to these modules, $node->file[FID]->inline, which tells these sorts of modules if people wnat that file to appear inline. AND it CAN (by default will) render a file in the most simple way inline. So really, if you want to -1 this patch, fine. But do not -1 it, because it does too little IYO. It still allows MUCH more than what you can do no, without that patch! And any solution, for core, that allows advanced handling of inline images will either require an enourmous amount of work, or it will simply no get in. So, unless you come up with a good core-worthy patch, this is still a big leap forward from what we have now. ------------------------------------------------------------------------ Sat, 23 Jul 2005 19:01:51 +0000 : Bèr Kessels :$ this is an attempt to fix the borken HML in the previous follow up. ------------------------------------------------------------------------ Sun, 24 Jul 2005 04:07:34 +0000 : TDobes Please don't combine unrelated changes together in the same patch. Moving stuff in/out of the _nodeapi hook has absolutely nothing to do with the patch and makes it much harder to read through the patch and see what it's actually doing. I'd appreciate it if you could separate the changes made here which are really relevant and move the other changes into another issue. Also: You change form_group_collapsible(t('File attachments') to form_group_collapsible(t('File Attachments'). A quick glance at some other core code seems to indicate that we've standardized on capitalizing only the first word in multi-word group titles. As for the functionality itself, I don't think this belongs in upload.module. It would be useful to have the functionality in core, but as a separate module which can be switched on and off. (upload_inline.module or something like that) If a site were set up using an inlining system from contrib and the functionality you're proposing could not be switched off, it would be extremely confusing to users that there are two different ways of inlining images. A minor comment: I'm not so sure whether the results of this patch, when inlining non-image attachments, will be the one users expect. A non-web-savvy person may expect his/her Word document (for instance) to appear inline with the node when they click "inline" -- just like their images do. I'd suggest that we limit inlining to images only. I'm not as adamant about this as the other issues, so I could be convinced otherwise if others disagree. ------------------------------------------------------------------------ Sun, 24 Jul 2005 07:58:19 +0000 : Bèr Kessels Thank you for your thoughts Tdobbes. As for the "moving things in and out of nodeapi" This was necessary, because otherwise the patch would have made the code even more unreadable. I agree with you on this point, but I simply had to clean it up a little, in order to be able to work in it. As it stands now, it is simply not possible to make this a separate module. We cannot hook into the files table. In a next stage, I might work out some hook for that table. But first things first. Can Dries or Steven please comment on his. I want to know If i should bother spending this enormous amount of time on commenting and debating this issue. If the chances are small this is accepted, Ill just drop it This ridiculous long thread reminds me again why I try to avoid making patches for core. ------------------------------------------------------------------------ Mon, 25 Jul 2005 05:49:37 +0000 : Steven Overall I like the concept, but I think it at least needs some default styling to make the image appear okay, like floating it. Right now it appears inline between whatever was there... very ugly. Sure you can say "leave it up to the themer" but if the goal is to provide a simple default, we can't expect people to dive into theming if they are going to use this feature instead of img_assist or whatever. Also, I wonder about the classes "upload" and "image". Is there a reason to use two classes? Isn't "image" a bit too generic? Oh and there is a missing XHTML / for the img tag. ------------------------------------------------------------------------ Mon, 25 Jul 2005 12:42:26 +0000 : Bèr Kessels I will fix the broken Xhttml. I chose "image" because it is exactly the same as the .image class used by image.module. But I will see to it, to add more functionality. I agree that it is not up to the themer for some good defaults, thus I will add a line of CSS to drupal.css (*shiver* ;) ) upload, IMO does not communicate well enough what the Image is doing. Not sure yet how I ill fix this, but I will see to it to come up with a better CSS/theme solution. Thanks! ------------------------------------------------------------------------ Tue, 26 Jul 2005 14:18:23 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_0.patch (12.24 KB) Suggestions by Steven are now included. I very simple float is added to the inline images in drupal.css. XHTML fixed. And I cleaned up some old crufty comments, as suggested by Dries. ------------------------------------------------------------------------ Tue, 26 Jul 2005 17:05:45 +0000 : fax8 What is proposed here is a good idea. But I think that this is done in not good way. I think that upload.module should be only a layer between file system and modules. So modify upload module to add a specific feature like your inline for me is not good. but I like the idea to add checkboxes for enable interaction modules between modules and upload.module. Maybe we could design an of api wich let modules tell upload.module that they can interact with it. I make an example: I'm a developer of video.module. Now to add a video file to video.module one need to put the file into drupal folders using ftp and then insert the path to the file during node creation. One of the features our users are asking a lot is the possibility to directly upload a video file to dupal. So I should write a lot of code, settings, security buggy code.. that for me isn't useful. What about if only calling an upload.module api I should enable a new checkbox on upload form called "use as video file" which let me use the file as a video file on my module??? What do you think about it???? ------------------------------------------------------------------------ Tue, 26 Jul 2005 20:00:03 +0000 : Bèr Kessels fax: upload /is/ such a layer. Just look at how image module does it. You need not write any additional 'buggy' code, you can use all teh upload functionality. The upload interface (the table and list), however, is tightly integrated, so that drupal offers an out-of-the-box file system. Nox, my patch simply adds an "out of the box" inline functionality. Small and simple. For any more advanced file features you should use or write a module that, in turn, uses the uplaod.module. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:22:40 +0000 : stefan nagtegaal After discussing with Ber, I think this patch has some great potential though the simplicity of it.. For the people who don't completely agree what this patch does: - It adds an extra checkbox for each attachment. If the 'Inline' checkbox is checked, images are displayed inline in your post and other attachments (like .pdf, .ppt, or whatever) were transformed into links. The potential of this patch is in the fact that this makes it much, much easier to have another contrib module which makes it possible to let you choose at the node submission how you would like your node to be displayed, incl. the selected inline attachments to be displayed (using _nodeapi()). So, a big +1 from me on this.. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:27:52 +0000 : Kobus > - It adds an extra checkbox for each attachment. If the 'Inline' > checkbox is checked, images are displayed inline in your post and other > attachments (like .pdf, .ppt, or whatever) were transformed into links. Does this mean it is done automatically or do I have control over this? I don't want the images to be inline if I can't control where I put it. Let me just clarify my position on this short-and-sweet-like: If it breaks the existing functionality that upload.module + inline.module provides without opening the door for a replacement, then I am -1, otherwise +1. :-) Kobus ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:51:50 +0000 : Bèr Kessels Kobus: Again: it does not break anything! It opens up enormous potential for modules such as inline/module. Because inline module now has access to a variable $node->files[fid]->inline. in other words: modules such as inline.module can now find out what images a user wants to see inline! ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:59:53 +0000 : Kobus Ber: Great, then I +1 it. From our previous talks about this, it seemed as if this functionality was being taken away. ------------------------------------------------------------------------ Wed, 27 Jul 2005 16:18:21 +0000 : Sohodojo Jim Agreed, +1. The most on-going and especially most recent discussion has clarified that we can have our simple cake and contrib mod cake as well. Does this mean that inline module will work as is, or will a post-patch update be required? --Sohodojo Jim-- ------------------------------------------------------------------------ Wed, 27 Jul 2005 17:33:47 +0000 : Bèr Kessels inline module will continue working. but in order for it to use this inline variable (and become even better) it needs to be patched. ------------------------------------------------------------------------ Thu, 28 Jul 2005 18:12:09 +0000 : moshe weitzman I read through the code and it looks good to me, though it is hard to know whats new given all the code thats moved around. this is definately needed functionality. If you were still up for enhancements, I'd like to see the 'inline' box get checked by default when uploading an image. when the attachment is not an image, the box should be disabled and unchecked. ------------------------------------------------------------------------ Thu, 28 Jul 2005 18:32:40 +0000 : Bèr Kessels disabling the box for non-images would break the link function (i.e add a link in the text. But, on second thought: that is fine, since it defeats the list checkbox. I will add that. As well as making it checked by default. ------------------------------------------------------------------------ Thu, 28 Jul 2005 19:54:14 +0000 : Junyor @Bèr: Why not simplify this patch to only add the hooks to a contrib module would need to add this functionality? You said that your patch would allow inline.module to work much more easily. But then there is a lot of extra stuff there that inline.module won't need. This functionality can't replace inline.module, so anything that it doesn't use just gets in the way. I'm afraid that users will think this is the best Drupal can provide without looking for a contrib module. I'd prefer we spent our time and effort adding the whole functionality of inline.module or making it easy to add and position inline content in nodes to core rather than adding some functionality to upload.module that doesn't cut it for most users. So, either strip this patch of everything but the functionality that inline.module (and similar modules) can use or allow me to position images in my text through this patch. Otherwise, -1. If this is the first step in a plan to add the functionality I want to core, I'd like to hear your full plan. ------------------------------------------------------------------------ Thu, 28 Jul 2005 21:19:37 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_1.patch (19.71 KB) to all: everyone has Big Plans[tm] and even worse: everyone has Bigger Plans for my patch, for inline module. Why not code it? Hooks? fine! module integration, even better. Sorry if I sound a bit grumpy here. But so far I have spent nearly three times as much hours debbating the patch, then I spent creating it. Hooks sound great. but are not the intention of this patch. I want to add a *default* way, that works. But that does not break other modules, nd in fact tries to help them. But I am not inventing some advanced image handling system here. Here is an updated patch with renewed updates.inc (got lost in the previous patch) Moshes suggestion about disabling non-images-checkboxes is also implemented. I like it a lot more now. I did not, however, implement Moshes idea about defaulting to "checked". for a few reasons: * It looks a bit silly if you have more then one image inline, And defaulting all uploads to inline will inline any uploaded image. * When uploading large images they will default to inline, thus pushing your sites layout to oblivian * None of the other options default to TRUE, so for consistancy I prefer to default it to not inline. ------------------------------------------------------------------------ Thu, 28 Jul 2005 21:24:26 +0000 : Bèr Kessels oh, bytheway: you can test adn play with this patch here: http://fixme.remixreading.org/?q=add_content note that it is a sandbox, so your account will get lost, and pages might not work :) ------------------------------------------------------------------------ Fri, 29 Jul 2005 03:47:35 +0000 : ejort +1 from me for this feature. It's frustrating that core doesn't include some basic image display functionality, and this will cover a large percentage of users needs without introducing complexity. People wanting arbitrary image placement still have inline.module etc., so it's nothing but an improvement on the current situation. ------------------------------------------------------------------------ Fri, 29 Jul 2005 06:38:46 +0000 : Steven Bèr: one thing is not clear to me... you are right that other modules can access the inline value. But upload.module will always add the image to the body. So, how can other modules use the inline checkbox/option meaningfully? ------------------------------------------------------------------------ Tue, 02 Aug 2005 18:33:30 +0000 : Bèr Kessels Steven: what I mean, is that they have access to a variable that tells them a user wants something inline. Nothing more. Its up to these modules to use that meaningfully. ------------------------------------------------------------------------ Thu, 01 Sep 2005 15:26:36 +0000 : Bèr Kessels The patch still applies. Dries, please let me know if I must bother trying to get this in before 4.7? ------------------------------------------------------------------------ Thu, 01 Sep 2005 15:30:09 +0000 : m3avrck Ber, check this latest commit: http://drupal.org/node/28483 ... I think your patch no longer applies since this JS based does do inline functionality. ------------------------------------------------------------------------ Thu, 01 Sep 2005 19:46:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_2.patch (20.02 KB) updated patch works with revisions and JS upload. ------------------------------------------------------------------------ Thu, 01 Sep 2005 20:32:11 +0000 : m3avrck got an extra 'favico' in the patch file there. ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:24 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_2_0.patch (19.22 KB) favicon removed. ------------------------------------------------------------------------ Thu, 01 Sep 2005 23:06:11 +0000 : killes(a)www.drop.org I think I'd like to have this feature in core. The patch is quite big, though. ;-/ Also I noticed that the pgsql part is bogus. ------------------------------------------------------------------------ Thu, 01 Sep 2005 23:27:42 +0000 : Bèr Kessels how would the pgsql look then? I am a complete pgSQL nitwit :) ------------------------------------------------------------------------ Fri, 02 Sep 2005 15:18:47 +0000 : telex4 Just a quick +1 from a user who is hoping that this gets into core for 4.7. We're using the inline functionality on our Remix Reading test site at the moment, and it's very handy for our theme where the user uploads any number of files and may want one or two images to be previewed in the node. ------------------------------------------------------------------------ Fri, 02 Sep 2005 17:26:49 +0000 : m3avrck Patch needs to be rerolled. Latest 2_0 version does indeed fix JS-based uploads commit, however it is missing all of the database changes and updates from the last _1 patch. Also, upload_count_size() is undefined, should be using upload_space_used() [2] instead. Also, beginning of upload.module patch adds the default maxsize setting which was previously taken out becuase it wasnt working properly. This patch [3] corrects that and is a seperate issue :) [2] http://drupaldocs.org/api/head/function/upload_space_used [3] http://drupal.org/node/30025 ------------------------------------------------------------------------ Fri, 02 Sep 2005 18:31:12 +0000 : Steven Bèr: I'm afraid you missed my point. It is pretty useless for other modules to be able to read the inline value, when upload.module always adds the image to the node body. Suppose I want to make a module which adds more advanced inline positioning, perhaps with a marker tag. I cannot avoid _upload_inline($node) being called, so I have to somehow remove that (themed!) image tag again. This makes the "other modules can use it" argument pretty silly IMO. ------------------------------------------------------------------------ Fri, 02 Sep 2005 18:48:32 +0000 : Dries It would make sense for Ber to provide one or two examples of how this could be used. ------------------------------------------------------------------------ Fri, 02 Sep 2005 21:02:39 +0000 : Bèr Kessels @Steven: no I did not miss your point. on nodeapi view this is appended to the $node->body, regardless. You are correct. But nothing witholds you from using that variable *before* the view is invoked. ou can then choose to unset the variable, or leave it. I first had yet another hook, that would be called on nodeapi view, by the upload, on a per-file base; but then I found that all this was very OTT, since it ca all be achieved with the existing nodeapi. I reverted to the nodeapi. @Dries; examples, sure. 1) Look for the result at http://fixme.remixreading.org/node/595, the small image at the right is rendered with this variable. 5and I must add that users that tested this behaviour were VERY positive!) In phptemplate, the node, we do: <?php foreach ($node->files as $file): ?> <?php if ($file->inline): ?> <div id="preview"> <img src="files/<?php print $file->filename ?>" alt="preview" title="preview of <?php print $node->title ?>" /> </div> <?php endif; ?> <?php endforeach; ?> Because we have ourtheme_upload_inline() to return NULL, this looks like you see online. Together with some CSS you get a very nice result. 2) For most weblogs, or newssites, you need not do anything, for the $node->body has the image already appended *if* the inline checkbox is checked. Esp since these types of sites need a unified look, they do not (or should not) need to alter the place and aligning of each image. 3) For the power users there are node-layout tools available such as tinyMCE, wikisyntax or even inline.module. Since that last one is the simplest I use that as example. <?php function inline_nodeapi(&$node, $op, $arg) { if(is_array($node->files) && $op == 'view') { //on view we replace the tokens with the img tags $node->teaser = _inline_substitute_tags($node, 'teaser'); $node->body = _inline_substitute_tags($node, 'body'); } elseif(is_array($node->files) && $op == 'validate') { $node = _inline_remove_tags($node); //remove the tags that are not flagged as inline } elseif(is_array($node->files) && $op == 'load') { _inline_remove_flags($node) //remove all $node->files[XX]->inline variables, so that upload does not render them. } elseif(is_array($node->files) && $op == 'from pre') { $node = _inline_add_tags($node); //add [inline:X] tokens to 'body', so editor can use them. $node = _inline_remove_tags($node); //remove [inline] tokens that call files that are (no longer) marked for inline use. } } //.... function _inline_add_tags(&$node, $field) { foreach ($node->files as $file) { if ($file->inline && !_inline_check_for_tag($file->filename)) { //look if the tag is not already in body, and if the file is marked for inline usage. $node->body = _inline_add_single_tag($file) . $node->body; //Actually add the token to the beginning of the body $node->teaser = _inline_add_single_tag($file) . $node->teaser; // same for teaser } } } ?> //note. preview is broken no Idea how this post looks ;) ------------------------------------------------------------------------ Sat, 03 Sep 2005 04:37:23 +0000 : TDobes Bèr: The idea of clearing $node->inline never occurred to me... but won't that only work some of the time? It depends on the order in which modules' _nodeapi hooks get called. I believe they get called alphebetically (there's an asort in module_list), so a module whose name starts with v, w, x, y, or z will not be able to prevent upload.module from displaying its inline image. While I do feel this functionality is useful, I think a better way of implementing it would be to allow other modules to add columns to the upload table. Then a module could invoke this upload hook to add a field for "display inline" and invoke _nodeapi to actually place the image. This would provide for future expansion of the upload module in addition to solving the immediate goal. ------------------------------------------------------------------------ Sat, 03 Sep 2005 06:37:17 +0000 : Bèr Kessels in short my battle plan was: * introduce inline stuff * introduce hook for altering the file table Why i did not go for #2 at once? because IMO you can do two things wit a file: show it, or download it. Anything else should be done in a file admin UI. And furthermore: yes modules are ran in alphabetic order, but, nodeapi(load) is ALWAYS ran before nodeapi(view). So you can always make sure on nodeapi(view) the variable is unset. ------------------------------------------------------------------------ Sat, 03 Sep 2005 12:22:00 +0000 : moshe weitzman I think that Ber's suggestion to unset the variable is a great, simple workaround. I don't see any more outstanding objections to the patch. Yes, it can be improved later, but it is useful already. ------------------------------------------------------------------------ Sat, 03 Sep 2005 13:08:05 +0000 : Bèr Kessels the patch is still in brokenish state. I am trying to free some time this weekend (sunday evening) to fix it and synch with HEAd again. not h revisions and JS upload broke this patch. Its a lot of work to reroll, due to that amount of changes (nearly a complete rewrite, I am afraid) ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:48:47 +0000 : Bèr Kessels please review. This a complete rewrite, * I ignored all the huge fucntions and just left tath for another ime to clean up (never). It does make the patch easier to read and smaller, though * I left the 'upload preview' bug for what it is: it belons in a differnt issue, and would clutter this inline feature patch a lot. For the rest see above. * Themes could be updated to do nice stuff to the inline patch. I did not do that, for that goes far beyond what this patch tries to do ------------------------------------------------------------------------ Mon, 12 Sep 2005 17:49:57 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_2_4.patch (14.13 KB) No, please review /this/ patch. ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:35:55 +0000 : m3avrck upload.module is listed twice in the patch ... so patch isn't work, please update! ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:17:38 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_2_5.patch (8.66 KB) last attempt ------------------------------------------------------------------------ Wed, 14 Sep 2005 17:50:24 +0000 : Souvent22 +1 Ber. Worked well. I agree, the upload preview issue belongs in a diff bug. perhaps i'll create one, i'll try and look into that bug today. but i really like the inline feature, and i like that it IS simple. nice. ------------------------------------------------------------------------ Wed, 14 Sep 2005 18:14:22 +0000 : Souvent22 Ber, As stated earlier about the bug, there seems to be a similar issue: http://drupal.org/node/31102 ------------------------------------------------------------------------ Wed, 14 Sep 2005 18:15:20 +0000 : Souvent22 Just wanted to state, this patch is ready to go. The "bug" is for all intents and purposes unrelated, and shall be resolved when the above mentioned issuse is resolved. I'd like to see this go in. ------------------------------------------------------------------------ Wed, 14 Sep 2005 19:23:56 +0000 : Souvent22 The problem with your upload file lists dissapearing has been resolved. http://drupal.org/node/31102

1 0

[drupal-devel] [feature] DHTML expandable menu links
by brevity 15 Sep '05

15 Sep '05

Issue status update for http://drupal.org/node/5943 Post a follow up: http://drupal.org/project/comments/add/5943 Project: Drupal -Version: cvs +Version: 4.6.3 Component: theme system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: flevour Updated by: brevity -Status: active +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/page.tpl.php.txt (5.14 KB) Based on the bluemarine template [1] for the PHP template engine and work by Nick [2] (example menu [3]), the following is a adaptation with working dynamic menu: page.tpl.php [4] Change the line reading print my_menu_tree(24); to your menu-id or to 0 to get a complete dynamic menu including administration menu items. -- Sorry for duplicate posting in http://drupal.org/node/31307. [1] http://drupal.org/project/bluemarine [2] http://www.nickrigby.com/article/25/drop-down-menus-horizontal-style-pt-3 [3] http://www.nickrigby.com/examples/dropdown4/index.htm [4] http://ten.ic24.net/drupal/page.tpl.php.txt brevity Previous comments: ------------------------------------------------------------------------ Thu, 19 Feb 2004 23:01:32 +0000 : flevour I was wondering if anyone has already worked out a patch to allow menus created by the menu() call to be dhtml-ly expandable. Most of the times its a waste of bandwidth to load 2/3 different pages to access the page you want. Best regards, flevour ------------------------------------------------------------------------ Fri, 20 Feb 2004 15:25:35 +0000 : flevour Setting as feature request. ------------------------------------------------------------------------ Mon, 23 Feb 2004 04:11:02 +0000 : ax i just implemented this [5] on my drupal site [6]. please have a look, see if it works with your browser, and tell me what you think of it. and feel free to make a patch for drupal (i won't have much time before next weekend). [5] http://drupal.kollm.org/node/view/36 [6] http://drupal.kollm.org/ ------------------------------------------------------------------------ Mon, 23 Feb 2004 04:49:54 +0000 : moshe weitzman Wonderful. This is an elegant, elegant implementation. I'd like to see menu.inc patched as ax suggests (possibly add a param to the function) so we can experiment with css menus. i think the javascript error on ax's site happens when the nav block is not shown. just an oversight i'm sure. ------------------------------------------------------------------------ Mon, 23 Feb 2004 13:59:27 +0000 : Anonymous Works fine here on Firebird 0.7. ------------------------------------------------------------------------ Tue, 24 Feb 2004 07:39:29 +0000 : ax thanks to everyone for testing. > i think the javascript error on ax's site happens when the nav block is not > shown. just an oversight i'm sure. right you are. i fixed that in the multimenu javascript [7]. regarding the half a second delay before the menu pops up, to save the user from disortientation suggested at drupal.kollm.org [8]: i guess this isn't possible with a pure css menu, but would require considerably more javascript. which /i/ almost certainly won't find / implement. feel free to suggest / implement anything related, though. what i could imagine is to make the menu look more like a real menu as on CSS Creator [9], ie. instead of expanding the submenus to the bottom, shift them out to the left / right. as i said before, this probably won't happen before the weekend. [7] http://drupal.kollm.org/themes/ax_clean/multimenu.js [8] http://drupal.kollm.org/node/view/36#comment [9] http://www.csscreator.com/menu/multimenu.php#vertm ------------------------------------------------------------------------ Fri, 27 Feb 2004 02:45:45 +0000 : Anonymous Indeed this would like nice with rollover menus like those on csscreator ------------------------------------------------------------------------ Sat, 13 Mar 2004 05:46:12 +0000 : moshe weitzman Attachment: http://drupal.org/files/issues/menu_show_all_items.patch (927 bytes) One line patch to menu_tree() so we can optionally show all links, not just active links. This is all the core code that needs to change in order to facilitate expandable menus. ------------------------------------------------------------------------ Sat, 13 Mar 2004 06:34:29 +0000 : gordon I have written my own module to create a dhtml navigation tree, by linking in phplayersmenu to build the tree menu and handle all the cross browser capiblities. I did have to copy most of menu_tree() to do this, but it meant that I didn't have to change the core. But it was not just menu_tree that has to be changed. remember that once you have made this change to useing dhtml menus you have access to every menu. So modules like the watchdog only display it's menus when you are in the admin section. There really needs to be a method to say to all modules that we want every menu, and done try to be a smart bugger. ------------------------------------------------------------------------ Thu, 18 Mar 2004 03:52:54 +0000 : moshe weitzman Gordon makes a good point. I think this patch is worthy despite this known limitation. ------------------------------------------------------------------------ Fri, 26 Mar 2004 06:40:49 +0000 : ax i put an updated version [10], implementing real csscreator style rollover menus and submenu indicators, on my site. please note the NOTE at the end. [10] http://drupal.kollm.org/node/view/38 ------------------------------------------------------------------------ Fri, 26 Mar 2004 09:46:58 +0000 : adrian Take a look at the following demo of the Archomai Transmenu drop down menu. It uses javascript, yes.. but it degrades well .. and doesnt require horribly kludged html I have tried it on firefox 0.7, ie 6.0 and opera 7 with exactly the same results. ------------------------------------------------------------------------ Fri, 26 Mar 2004 14:09:39 +0000 : ax > Take a look at the following demo of the Archomai Transmenu drop down > menu. nice menu, nice theme. thats phptemplate [11], isn't it? > It uses javascript, yes.. but it degrades well .. and doesnt require > horribly kludged html the nice thing about the CSS Creator menu [12] is that for non-IE browsers, it works *without any javascript*, with css only. and the javascript to make it work in IE is just some 10 lines. besides that, it degrades as well and doesn't require kludged html, neither. i'm sure your demo can be done the same as nice with the CSS Creator menu. mind to give it a try? [11] http://drupal.org/node/view/6711 [12] http://www.csscreator.com/menu/multimenu.php ------------------------------------------------------------------------ Fri, 26 Mar 2004 14:33:49 +0000 : moshe weitzman One more benefit of the CSSCreator scheme is that the HTML is much easier to construct. The brief javascript snippet in CSSCreator figures out which items have submenus and applies the proper styling accordingly. The Anarchai menu on the other hand requires the user to embed classes in his HTML depending on whether they have children or not. Thats harder to contruct. An example from Adrian's site example, <?php <li class="firstItem collapsed">capdev ?> ------------------------------------------------------------------------ Thu, 15 Apr 2004 19:30:18 +0000 : JonBob When the admin-customizable menu patch is applied (http://drupal.org/node/view/7172) this should be possible to do without patching core. The whole menu tree will be available at all times to modules. ------------------------------------------------------------------------ Sun, 18 Apr 2004 15:41:08 +0000 : Dries Has anyone tried using expandable menus after JonBob's menu changes have been committed? According to JonBob this patch is no longer required ... ------------------------------------------------------------------------ Sat, 24 Apr 2004 15:54:14 +0000 : Dries I'm marking this fixed as I believe this is no longer an issue now JonBob's menu system improvements hit CVS. Please reopen if necessary. ------------------------------------------------------------------------ Sat, 24 Apr 2004 16:02:17 +0000 : moshe weitzman The new menu system makes this job possible, but still difficult. I think the patch proposed here is still a valid approach. What the UI designer needs is for the menu system to output a UL. menu_get_menu() does not do this. menu_get_tree() does, but it omits invisible items. Thus, I still propose we enhance menu_get_tree() to optionally include hidden items. ------------------------------------------------------------------------ Tue, 27 Jul 2004 14:17:45 +0000 : JonBob In HEAD, theme('menu_tree', 1, TRUE) will return a rendered tree of all accessible menu items in the main nav menu. ------------------------------------------------------------------------ Fri, 11 Feb 2005 15:19:11 +0000 : moshe weitzman Perhaps some entrprising theme designer wants to apply some DHTML menu renderer to theme('menu_tree', 1, TRUE), the function cited by JonBob. The output of that function shoudl work with most smart menu scripts, since its just a UL ------------------------------------------------------------------------ Fri, 15 Jul 2005 21:25:23 +0000 : lekei Since this thread was before 4.6 -- is "head" referred to here 4.6 or was all the work I did building a template with DHTML menus a waste? I need to launch this site, do I need to remove the menus from the template? I didn't think that an apparently closed feature request was the right place to ask, but I go to response trying to ask in what I thought was a more appropriate location. ------------------------------------------------------------------------ Thu, 11 Aug 2005 16:11:22 +0000 : ax hm - seems that theme('menu_tree', 1, TRUE) doesn't work anymore? apparently, it silently disappeared between revision 1.77 and 1.78 [13] as part of this patch [14]. r1.78 was the penultimate change before DRUPAL-4-6 [15], which means that theme('menu_tree', 1, TRUE) is /not/ in Drupal 4.6. bad bad bad. i rely on this feature for a dynamic menu on certain sites of mine. was the patch supposed to remove it ("Make local tasks more themeable" doesn't suggest this)? is there another way to get an completely expanded menu tree with proper menu item class labeling ("expanded", "collapsed") for the current menu path? if not, could this functionality please be reinstated? thanks for any insights. [13] http://cvs.drupal.org/viewcvs/drupal/drupal/includes/menu.inc?r1=1.77&r2=1.… [14] http://drupal.org/node/17869 [15] http://cvs.drupal.org/viewcvs/drupal/drupal/includes/menu.inc#rev1.79 ------------------------------------------------------------------------ Fri, 09 Sep 2005 09:50:24 +0000 : lekei So I take this to mean that Drupal can't do common DHTML menus, or any menu structure other than the primitive repaint-the-entire-page-on-each-mouse-click menus? The only way to add items to a dynamic menu seems to be to hand-code them into the template one page at a time. ------------------------------------------------------------------------ Fri, 09 Sep 2005 12:53:36 +0000 : brevity Using menu_get_menu from http://drupaldocs.org/api/4.6/group/menu should help to hand over a complete menu to some JavaScript, no? Something along that lines in your phpTemplate/theme... <?php function my_menu_tree($pid = 1) { $menu = menu_get_menu(); $output = ; { foreach ($menu['visible'][$pid]['children'] as $mid) { print theme_menu_item($mid); // print out current menu-item; change to fit your dHtml/JS code? my_menu_tree($mid); // recursive call to get sub-menu-items } } return $output; } ?> ... combined with http://www.nickrigby.com/examples/dropdown4/index.htm from http://www.nickrigby.com/article/25/drop-down-menus-horizontal-style-pt-3 and I would be happy, too. ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:38:55 +0000 : Tobias Maier not print theme_menu_item($mid); // print out current menu-item; change to fit your dHtml/JS code? better print theme('menu_item', $mid); the function theme_menu_item is just the fall back function if no such function is available in the theme!

1 0