January 2005 - Development - lists.drupal.org

[drupal-devel] [task] Put revisions in their own table
by Dries 31 Jan '05

31 Jan '05

Project: Drupal Version: cvs Component: node system Category: tasks Priority: normal Assigned to: killes(a)www.drop.org Reported by: killes(a)www.drop.org Updated by: Dries Status: patch Anyone to help review/test this? Dries Previous comments: ------------------------------------------------------------------------ May 5, 2004 - 18:25 : killes(a)www.drop.org Currently all node revisions are stored in a serialized field in node.table and retrieved for _each_ page view although they are rarely needed. However, we have agreed that serializing data is bad and that we should try to keep the memory foot print pf Drupal small. Therefore I propose to create a separate revisions table which would be in principle identical to the node table, only that it could have several old copies of the same node. Extra data added by other modules could be added in a serialized field unless we find a better solution. ------------------------------------------------------------------------ May 5, 2004 - 19:06 : jhriggs I too think the serialized approach is less than desirable, but here's an alternative. This would likely take some considerable rework in core and contrib, but the following is how we handle similar types of situations in our databases at work. It is more elegant that a separate table, and avoids the (almost exact) duplication of a table. Instead of separate tables, keep all revisions of nodes in the node table as follows: * add field: active (0/1 or Y/N) * add field: revision * every revision of a node is stored in the node table; however, only one revision can be active at any given time * nid can no longer be unique -- primary/unique key becomes (nid, active) * any time a node is loaded, updated (without revision), etc., the active version is used. Thoughts? ------------------------------------------------------------------------ May 5, 2004 - 19:57 : killes(a)www.drop.org I am not opposed to your scheme, but I want to stress the following: * Duplicating a table's structure is not bad (IMHO) as long as the content is different. * having two tables will allow us to have a rather small node table. This is (maybe) a performance gain. ------------------------------------------------------------------------ May 5, 2004 - 20:37 : jhriggs I don't necessarily think that duplicating a table's structure is _bad_. It just seems to be wasteful and a pain to maintain. (Every change to the node table is made twice...easy to do, but also easy to miss perhaps.) As for performance, as long as nid and the active indicator are indexed, there shouldn't be any performance loss. Also, archiving an old version when making a new revision will be much simpler: just change the active indicator rather than copying an entire node to another table (and ensuring everything gets copied...again a potential maintainance issue). To be honest, I would just like to see the serialized data go away, regarless of what approach is taken. ------------------------------------------------------------------------ July 30, 2004 - 21:49 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_07-30-2004.p… (10.47 KB) I'm interested in using Drupal for a large scale wiki-type project. In order to do this, I need revisions to be in their own table. Attached is a patch to do just that. Most of the changes are pretty self explanatory. Spreading out node data across two tables meant that I had to add database functions to do locking/transactions. Without this, race conditions in which the database becomes corrupted are possible. ------------------------------------------------------------------------ July 30, 2004 - 21:54 : Nick Nassar Oh yeah... The patch is a diff against Drupal CVS ------------------------------------------------------------------------ July 31, 2004 - 02:00 : Anonymous Gerhard speaking. Nick, thanks a lot for your nice patch! It saves me a great deal of labour. I looked through it and immediately liked it. You not only put the old revisions into a new table but also the current one. Do you have an estimate how much more expensive the additional join is? Besides a few minor coding style issues I found a major one: Just a few hours before you uploaded your patch JonBob's node access patch hit core. That means your patch won't apply anymore as all the queries you change have been changed. Can I bug you to update your patch? ------------------------------------------------------------------------ July 31, 2004 - 03:11 : Anonymous Also I think that your upgrade path loses existing revisions. ------------------------------------------------------------------------ July 31, 2004 - 04:39 : drumm I think this is the proper way to do things. No columns are duplicated, there is no serialized data, and only the fields that are logically revised are stored. Nothing jumped out at me as a way to have my node modules be able to keep a table of revisions of additional fields. I'm guessing this could be done within the confines of _insert and _update. Assuming the upgrade path works and modules can extend it I give it a +1. ------------------------------------------------------------------------ July 31, 2004 - 16:40 : Nick Nassar It figures that just as I finish a big patch, another patch comes along and breaks it. Oh well, it should be a pretty easy to fix. I'll work on it. Fixing the upgrade path to keep revisions should be fairly painless. I found another issue that needs to be fixed before this patch gets merged. There format of a node needs to be stored for each revision. Otherwise, for modules that store a format for the nodes, such as page and book, if you write one revision in PHP and the next in HTML, the PHP revision will be displayed as HTML. This is part of a larger issue of how node modules should store revisions of additional fields. I think each module that wants to do this should create another table with (nid, revid) as the primary key. Just as when they want to add fields to a node they create another table with nid as the primary key. As far as performance goes, for sites that make heavy use of revisions, an extra join on primary keys is going to be a lot faster than grabbing all of the revisions from that database everytime. We would need to run benchmarks to determine is the overall difference in speed is for an average site is a gain or a loss. I'm guessing it's very minor either way. ------------------------------------------------------------------------ August 23, 2004 - 15:55 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_08-23-2004.p… (10.92 KB) Here's an updated patch against CVS that puts revisions in their own table, provides an upgrade path, and fixes the format related bugs in the last patch. Hopefully, this can make it into CVS as soon as the freeze is over. ------------------------------------------------------------------------ August 23, 2004 - 16:10 : moshe weitzman Interesting patch ... drumm's question is still outstanding. how do modules store revisions of their fields? Are they expected to manage this on their own? Thats not how it works today. As an aside, i am seeing profile_ fields in my node.revisions column. One could argue that those need not be saved. They pertain to the node author, not to the node itself. ------------------------------------------------------------------------ August 23, 2004 - 18:14 : Nick Nassar Having modules be responsible for storing revisions of their own fields is a side-effect of storing revision data in tables. There's really no way around it. However, revisions generally don't make sense for node types that don't have PHP/HTML content, such as polls. I think it's going to be a pretty rare scenario for a new node type to want another field to change per-revision, so it's a pretty good trade-off. Storing fields that shouldn't be part of revisions, such as the profile_ fields, is a side-effect of storing revisions as serialized objects. Applying this patch will free up that wasted space. :) ------------------------------------------------------------------------ August 23, 2004 - 19:20 : Anonymous There should be a hook that let's the module choose whether it supports history. This way a module author can prevent the user from doing something that may break his module or just cause undefined behavior. If the module doesn't support history then don't let the user/admin choose to add history to nodes of that type. Craig ------------------------------------------------------------------------ August 23, 2004 - 21:23 : Nick Nassar I agree, there should be an API change to make specifying support for revisions easier. In the interests of keeping patches small and keeping to one change per patch, I think the API change should be a separate issue. A sort of ad-hoc API to decide whether or not a module supports revisions by default already exists. Instead of having a hook, modules set the default value of the "Create new revision" field in the edit form. The admin can change this option in admin/node/configure/defaults. This patch doesn't change that. Revisions are broken for node types that have their own database structure, like polls, even when storing them as serialized objects. This patch doesn't change that, either. ------------------------------------------------------------------------ October 26, 2004 - 04:35 : moshe weitzman I'm guessing that someone is going to have to demonstrate that this patch performs as well as current drupal before it gets comitted. i think this patch is a few benchmarks from being comitted. ------------------------------------------------------------------------ October 27, 2004 - 03:04 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB) I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ October 27, 2004 - 03:05 : Nick Nassar I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ October 27, 2004 - 03:05 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB) I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ November 15, 2004 - 07:05 : elias1884 please overthink the revision system default workflow as well. don't look at the revision system as an isolated system but as a part of the whole workflow system! if you combine revisions with the moderation queue the most logic default workflow would be like that: auth user creates node (revision #0) admin approves the node (status = 1, moderation = 0) => node publicly available auth user finds typo and changes node (revision #1, status = 0, moderation = 1) ------- what happens at that point at the moment is, that the node is not accessible anymore at all until the new revision is approved by admin. of course the new revision should not go online until reviewed and approved, this is absolutely correct, but there is no reason to not take the old revision offline, since it was already approved and should therefore be online until the new revision is approved. it is not practical if a node disappears only because the author corrected a typo. ------- admin approves the node (status = 1, moderation = 0) eventhough I first thought a plain boolean active field would not be capable of providing that functionality if finally came to the conclusion, that it can. The only thing to do is to not set that bit, when a new revision is created, but when it is approved (in case moderation is activated under default workflow). Every revision should have its own moderation, status and active field and on approval they are set like this (status=1, moderation=0, active=Y). When you wanna rollback to an old revision, you can chose between all revisions that already have the moderation bit set back to 0 again and the published to 1. There should be an extra permission for rollback! another concern that I have about the default workflow is, that users can't see the content, they have just created, when moderation is enabled. Eventhough, there is a big fat "submission accepted" presented after submissions, unexperienced users tend to question the information those stupid tincans give them, if they can't find their content afterwards. Many users are really lazy bastards and they don't even read the status messages. The best feedback about whether his story was submitted successfully or not of course is, if he can find the story somewhere on the site, maybe with a status message on top of it, mentioning, that the content is currently not publicly available since it has not been approved yet. there should be a my content section under my account, like somebody is trying to do with the workspace module I guess. so my suggestion is to make (status=0, moderation=1) still available for the creator under a my content section somewhere! ------------------------------------------------------------------------ November 24, 2004 - 06:21 : Nick Nassar I agree. The current workflow for moderation queues and revisions needs to change, but this patch isn't the place for it. The patch is already too big, and it only does the backend stuff. Instead of adding more to this patch and making it take even longer to get into core, would you mind creating a new issue for your UI suggestions, so the those changes can be added as a separate patch? Thanks, Nick ------------------------------------------------------------------------ December 11, 2004 - 14:26 : Dries This patch is _much_ needed so I'd love to see someone revive it. In order for this patch to be accepted, the following needs to be done: Update this patch to CVS HEAD. Rename revid to vid. Rename node_rev to node_revisions. Rename node_rev.changed to node_revisions.timestamp. Rename $rnode to $revision. Fix the coding style to match Drupal's: proper spacing, single quotes where possible, proper variable names. Benchmark this patch with a large database with enough revisions. I'd be happy to benchmark this on my local copy of the drupal.org database. The book.log field should probably move to the node_revisions table. This can be done in a separate patch. Investigate whether transactions are well-supported. ------------------------------------------------------------------------ December 13, 2004 - 02:25 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004-r… (11.02 KB) I've worked a bit on the patch (coding style issues as mentioned by Dries). One thing I noticed is that the patch uses REPLACE. IIRC this needs to be chagned to "UPDATE, if fail INSERT" for pgsql compatibility. Nick, are you still interested in working on that patch? I'd like to know how it works on your site and work on getting it into core. ------------------------------------------------------------------------ December 13, 2004 - 14:33 : Dries Gerhard: your patch does not apply. ------------------------------------------------------------------------ December 13, 2004 - 19:10 : killes(a)www.drop.org Yes, I know, that was the same version as I mailed to you earlier. ------------------------------------------------------------------------ December 13, 2004 - 23:02 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions.patch (52.96 KB) Ok, upüdated the patch to cvs. ------------------------------------------------------------------------ December 14, 2004 - 10:58 : Dries Some more comments: db_begin_transaction() and db_end_transaction() do not belong in database.inc, but in database.mysql.inc and database.pgsql.inc respectively. The node module calls node_revisionsision_list() which is not defined. (Fxed that on my local copy.) Do db_begin_transaction() and db_end_transaction() deprecate Jeremy's table locking patch? The upgrade path assigns the wrong user ID to each revision. The upgrade path assigns the wrong date to each revision (that or a node's revision page shows the wrong usernames/dates). The coding style needs a bit of work, but we can worry about that later. ------------------------------------------------------------------------ December 14, 2004 - 19:34 : Nick Nassar If you need any help getting those things fixed, just let me know. ------------------------------------------------------------------------ December 14, 2004 - 19:50 : Nick Nassar How this relates to Jeremy's node locking patch: There was lots of discussion, and node locking was decided against because from an end user point of view you never want a node to be locked. He's now advocating for a much simpler patch that warns users if their changes will overwrite someone elses. That patch still has a race condition, which might be fixed using db_begin_transaction(). http://drupal.org/node/6025 ------------------------------------------------------------------------ December 15, 2004 - 00:26 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_0.patch (55.96 KB) Here is an updated patch that tries to address Dries concerns. ------------------------------------------------------------------------ December 15, 2004 - 10:32 : Dries Attachment: http://drupal.org/files/issues/revisions-bug.png (76.06 KB) It didn't fix the aforementioned bugs. See attached screenshot. ------------------------------------------------------------------------ January 6, 2005 - 22:15 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_1.patch (51.77 KB) Ok, here is a new version. Dries and myself worked hart at it, so please have a look. what is still missing - database upgrades for the core modules with an own table - contrib modules need an upgrade too. - do we need nid and vid in both the node and the node_revisions table? - the amount of sql queries means a good stress testing for large databases. ------------------------------------------------------------------------ January 19, 2005 - 23:43 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_2.patch (49.49 KB) Here is an updated patch. We discussed to keep the current title in node module and also in the revisiosn table. This is content duplication but will save many joins as many queries only need the title of a node. Discussion is welcome. ------------------------------------------------------------------------ January 20, 2005 - 01:33 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_3.patch (29.93 KB) I've implemented the aforementioned solution. This makes the patch much smaller. The patch now also removes taxonomy_node_has_term() which wasn't used anywhere. I'd really apprciate if some people could test drive the patch. It will be another huge improvement for 4.6. ------------------------------------------------------------------------ January 20, 2005 - 02:05 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_4.patch (30 KB) Another revision. Steven didn't like my literal $node->vid in queries. ------------------------------------------------------------------------ January 20, 2005 - 03:10 : killes(a)www.drop.org - database upgrades for the core modules with an own table - contrib modules need an upgrade too. - do we need nid and vid in both the node and the node_revisions table? - the amount of sql queries means a good stress testing for large databases. These issues are still open, btw. Especially the first one needs to be tackled. ------------------------------------------------------------------------ January 25, 2005 - 22:11 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_5.patch (51.13 KB) Here is a patch that has the database tables updated for forum, book, and page module. ------------------------------------------------------------------------ January 30, 2005 - 00:55 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_6.patch (49.18 KB) Yet another update to keep it working with head. The patch now also removes the table definitons for the page table. ------------------------------------------------------------------------ January 30, 2005 - 00:57 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_7.patch (55.69 KB) Sorry, that was the old version, this is the right one. ------------------------------------------------------------------------ January 31, 2005 - 21:55 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_8.patch (55.71 KB) Updated once more. -- View: http://drupal.org/node/7582 Edit: http://drupal.org/project/comments/add/7582

1 0

[drupal-devel] [task] Put revisions in their own table
by killes 31 Jan '05

31 Jan '05

Project: Drupal Version: cvs Component: node system Category: tasks Priority: normal Assigned to: killes(a)www.drop.org Reported by: killes(a)www.drop.org Updated by: killes(a)www.drop.org Status: patch Attachment: http://drupal.org/files/issues/revisions_8.patch (55.71 KB) Updated once more. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ May 5, 2004 - 17:25 : killes(a)www.drop.org Currently all node revisions are stored in a serialized field in node.table and retrieved for _each_ page view although they are rarely needed. However, we have agreed that serializing data is bad and that we should try to keep the memory foot print pf Drupal small. Therefore I propose to create a separate revisions table which would be in principle identical to the node table, only that it could have several old copies of the same node. Extra data added by other modules could be added in a serialized field unless we find a better solution. ------------------------------------------------------------------------ May 5, 2004 - 18:06 : jhriggs I too think the serialized approach is less than desirable, but here's an alternative. This would likely take some considerable rework in core and contrib, but the following is how we handle similar types of situations in our databases at work. It is more elegant that a separate table, and avoids the (almost exact) duplication of a table. Instead of separate tables, keep all revisions of nodes in the node table as follows: * add field: active (0/1 or Y/N) * add field: revision * every revision of a node is stored in the node table; however, only one revision can be active at any given time * nid can no longer be unique -- primary/unique key becomes (nid, active) * any time a node is loaded, updated (without revision), etc., the active version is used. Thoughts? ------------------------------------------------------------------------ May 5, 2004 - 18:57 : killes(a)www.drop.org I am not opposed to your scheme, but I want to stress the following: * Duplicating a table's structure is not bad (IMHO) as long as the content is different. * having two tables will allow us to have a rather small node table. This is (maybe) a performance gain. ------------------------------------------------------------------------ May 5, 2004 - 19:37 : jhriggs I don't necessarily think that duplicating a table's structure is _bad_. It just seems to be wasteful and a pain to maintain. (Every change to the node table is made twice...easy to do, but also easy to miss perhaps.) As for performance, as long as nid and the active indicator are indexed, there shouldn't be any performance loss. Also, archiving an old version when making a new revision will be much simpler: just change the active indicator rather than copying an entire node to another table (and ensuring everything gets copied...again a potential maintainance issue). To be honest, I would just like to see the serialized data go away, regarless of what approach is taken. ------------------------------------------------------------------------ July 30, 2004 - 20:49 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_07-30-2004.p… (10.47 KB) I'm interested in using Drupal for a large scale wiki-type project. In order to do this, I need revisions to be in their own table. Attached is a patch to do just that. Most of the changes are pretty self explanatory. Spreading out node data across two tables meant that I had to add database functions to do locking/transactions. Without this, race conditions in which the database becomes corrupted are possible. ------------------------------------------------------------------------ July 30, 2004 - 20:54 : Nick Nassar Oh yeah... The patch is a diff against Drupal CVS ------------------------------------------------------------------------ July 31, 2004 - 01:00 : Anonymous Gerhard speaking. Nick, thanks a lot for your nice patch! It saves me a great deal of labour. I looked through it and immediately liked it. You not only put the old revisions into a new table but also the current one. Do you have an estimate how much more expensive the additional join is? Besides a few minor coding style issues I found a major one: Just a few hours before you uploaded your patch JonBob's node access patch hit core. That means your patch won't apply anymore as all the queries you change have been changed. Can I bug you to update your patch? ------------------------------------------------------------------------ July 31, 2004 - 02:11 : Anonymous Also I think that your upgrade path loses existing revisions. ------------------------------------------------------------------------ July 31, 2004 - 03:39 : drumm I think this is the proper way to do things. No columns are duplicated, there is no serialized data, and only the fields that are logically revised are stored. Nothing jumped out at me as a way to have my node modules be able to keep a table of revisions of additional fields. I'm guessing this could be done within the confines of _insert and _update. Assuming the upgrade path works and modules can extend it I give it a +1. ------------------------------------------------------------------------ July 31, 2004 - 15:40 : Nick Nassar It figures that just as I finish a big patch, another patch comes along and breaks it. Oh well, it should be a pretty easy to fix. I'll work on it. Fixing the upgrade path to keep revisions should be fairly painless. I found another issue that needs to be fixed before this patch gets merged. There format of a node needs to be stored for each revision. Otherwise, for modules that store a format for the nodes, such as page and book, if you write one revision in PHP and the next in HTML, the PHP revision will be displayed as HTML. This is part of a larger issue of how node modules should store revisions of additional fields. I think each module that wants to do this should create another table with (nid, revid) as the primary key. Just as when they want to add fields to a node they create another table with nid as the primary key. As far as performance goes, for sites that make heavy use of revisions, an extra join on primary keys is going to be a lot faster than grabbing all of the revisions from that database everytime. We would need to run benchmarks to determine is the overall difference in speed is for an average site is a gain or a loss. I'm guessing it's very minor either way. ------------------------------------------------------------------------ August 23, 2004 - 14:55 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_08-23-2004.p… (10.92 KB) Here's an updated patch against CVS that puts revisions in their own table, provides an upgrade path, and fixes the format related bugs in the last patch. Hopefully, this can make it into CVS as soon as the freeze is over. ------------------------------------------------------------------------ August 23, 2004 - 15:10 : moshe weitzman Interesting patch ... drumm's question is still outstanding. how do modules store revisions of their fields? Are they expected to manage this on their own? Thats not how it works today. As an aside, i am seeing profile_ fields in my node.revisions column. One could argue that those need not be saved. They pertain to the node author, not to the node itself. ------------------------------------------------------------------------ August 23, 2004 - 17:14 : Nick Nassar Having modules be responsible for storing revisions of their own fields is a side-effect of storing revision data in tables. There's really no way around it. However, revisions generally don't make sense for node types that don't have PHP/HTML content, such as polls. I think it's going to be a pretty rare scenario for a new node type to want another field to change per-revision, so it's a pretty good trade-off. Storing fields that shouldn't be part of revisions, such as the profile_ fields, is a side-effect of storing revisions as serialized objects. Applying this patch will free up that wasted space. :) ------------------------------------------------------------------------ August 23, 2004 - 18:20 : Anonymous There should be a hook that let's the module choose whether it supports history. This way a module author can prevent the user from doing something that may break his module or just cause undefined behavior. If the module doesn't support history then don't let the user/admin choose to add history to nodes of that type. Craig ------------------------------------------------------------------------ August 23, 2004 - 20:23 : Nick Nassar I agree, there should be an API change to make specifying support for revisions easier. In the interests of keeping patches small and keeping to one change per patch, I think the API change should be a separate issue. A sort of ad-hoc API to decide whether or not a module supports revisions by default already exists. Instead of having a hook, modules set the default value of the "Create new revision" field in the edit form. The admin can change this option in admin/node/configure/defaults. This patch doesn't change that. Revisions are broken for node types that have their own database structure, like polls, even when storing them as serialized objects. This patch doesn't change that, either. ------------------------------------------------------------------------ October 26, 2004 - 03:35 : moshe weitzman I'm guessing that someone is going to have to demonstrate that this patch performs as well as current drupal before it gets comitted. i think this patch is a few benchmarks from being comitted. ------------------------------------------------------------------------ October 27, 2004 - 02:04 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB) I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ October 27, 2004 - 02:05 : Nick Nassar I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ October 27, 2004 - 02:05 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB) I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ November 15, 2004 - 06:05 : elias1884 please overthink the revision system default workflow as well. don't look at the revision system as an isolated system but as a part of the whole workflow system! if you combine revisions with the moderation queue the most logic default workflow would be like that: auth user creates node (revision #0) admin approves the node (status = 1, moderation = 0) => node publicly available auth user finds typo and changes node (revision #1, status = 0, moderation = 1) ------- what happens at that point at the moment is, that the node is not accessible anymore at all until the new revision is approved by admin. of course the new revision should not go online until reviewed and approved, this is absolutely correct, but there is no reason to not take the old revision offline, since it was already approved and should therefore be online until the new revision is approved. it is not practical if a node disappears only because the author corrected a typo. ------- admin approves the node (status = 1, moderation = 0) eventhough I first thought a plain boolean active field would not be capable of providing that functionality if finally came to the conclusion, that it can. The only thing to do is to not set that bit, when a new revision is created, but when it is approved (in case moderation is activated under default workflow). Every revision should have its own moderation, status and active field and on approval they are set like this (status=1, moderation=0, active=Y). When you wanna rollback to an old revision, you can chose between all revisions that already have the moderation bit set back to 0 again and the published to 1. There should be an extra permission for rollback! another concern that I have about the default workflow is, that users can't see the content, they have just created, when moderation is enabled. Eventhough, there is a big fat "submission accepted" presented after submissions, unexperienced users tend to question the information those stupid tincans give them, if they can't find their content afterwards. Many users are really lazy bastards and they don't even read the status messages. The best feedback about whether his story was submitted successfully or not of course is, if he can find the story somewhere on the site, maybe with a status message on top of it, mentioning, that the content is currently not publicly available since it has not been approved yet. there should be a my content section under my account, like somebody is trying to do with the workspace module I guess. so my suggestion is to make (status=0, moderation=1) still available for the creator under a my content section somewhere! ------------------------------------------------------------------------ November 24, 2004 - 05:21 : Nick Nassar I agree. The current workflow for moderation queues and revisions needs to change, but this patch isn't the place for it. The patch is already too big, and it only does the backend stuff. Instead of adding more to this patch and making it take even longer to get into core, would you mind creating a new issue for your UI suggestions, so the those changes can be added as a separate patch? Thanks, Nick ------------------------------------------------------------------------ December 11, 2004 - 13:26 : Dries This patch is _much_ needed so I'd love to see someone revive it. In order for this patch to be accepted, the following needs to be done: Update this patch to CVS HEAD. Rename revid to vid. Rename node_rev to node_revisions. Rename node_rev.changed to node_revisions.timestamp. Rename $rnode to $revision. Fix the coding style to match Drupal's: proper spacing, single quotes where possible, proper variable names. Benchmark this patch with a large database with enough revisions. I'd be happy to benchmark this on my local copy of the drupal.org database. The book.log field should probably move to the node_revisions table. This can be done in a separate patch. Investigate whether transactions are well-supported. ------------------------------------------------------------------------ December 13, 2004 - 01:25 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004-r… (11.02 KB) I've worked a bit on the patch (coding style issues as mentioned by Dries). One thing I noticed is that the patch uses REPLACE. IIRC this needs to be chagned to "UPDATE, if fail INSERT" for pgsql compatibility. Nick, are you still interested in working on that patch? I'd like to know how it works on your site and work on getting it into core. ------------------------------------------------------------------------ December 13, 2004 - 13:33 : Dries Gerhard: your patch does not apply. ------------------------------------------------------------------------ December 13, 2004 - 18:10 : killes(a)www.drop.org Yes, I know, that was the same version as I mailed to you earlier. ------------------------------------------------------------------------ December 13, 2004 - 22:02 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions.patch (52.96 KB) Ok, upüdated the patch to cvs. ------------------------------------------------------------------------ December 14, 2004 - 09:58 : Dries Some more comments: db_begin_transaction() and db_end_transaction() do not belong in database.inc, but in database.mysql.inc and database.pgsql.inc respectively. The node module calls node_revisionsision_list() which is not defined. (Fxed that on my local copy.) Do db_begin_transaction() and db_end_transaction() deprecate Jeremy's table locking patch? The upgrade path assigns the wrong user ID to each revision. The upgrade path assigns the wrong date to each revision (that or a node's revision page shows the wrong usernames/dates). The coding style needs a bit of work, but we can worry about that later. ------------------------------------------------------------------------ December 14, 2004 - 18:34 : Nick Nassar If you need any help getting those things fixed, just let me know. ------------------------------------------------------------------------ December 14, 2004 - 18:50 : Nick Nassar How this relates to Jeremy's node locking patch: There was lots of discussion, and node locking was decided against because from an end user point of view you never want a node to be locked. He's now advocating for a much simpler patch that warns users if their changes will overwrite someone elses. That patch still has a race condition, which might be fixed using db_begin_transaction(). http://drupal.org/node/6025 ------------------------------------------------------------------------ December 14, 2004 - 23:26 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_0.patch (55.96 KB) Here is an updated patch that tries to address Dries concerns. ------------------------------------------------------------------------ December 15, 2004 - 09:32 : Dries Attachment: http://drupal.org/files/issues/revisions-bug.png (76.06 KB) It didn't fix the aforementioned bugs. See attached screenshot. ------------------------------------------------------------------------ January 6, 2005 - 21:15 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_1.patch (51.77 KB) Ok, here is a new version. Dries and myself worked hart at it, so please have a look. what is still missing - database upgrades for the core modules with an own table - contrib modules need an upgrade too. - do we need nid and vid in both the node and the node_revisions table? - the amount of sql queries means a good stress testing for large databases. ------------------------------------------------------------------------ January 19, 2005 - 22:43 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_2.patch (49.49 KB) Here is an updated patch. We discussed to keep the current title in node module and also in the revisiosn table. This is content duplication but will save many joins as many queries only need the title of a node. Discussion is welcome. ------------------------------------------------------------------------ January 20, 2005 - 00:33 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_3.patch (29.93 KB) I've implemented the aforementioned solution. This makes the patch much smaller. The patch now also removes taxonomy_node_has_term() which wasn't used anywhere. I'd really apprciate if some people could test drive the patch. It will be another huge improvement for 4.6. ------------------------------------------------------------------------ January 20, 2005 - 01:05 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_4.patch (30 KB) Another revision. Steven didn't like my literal $node->vid in queries. ------------------------------------------------------------------------ January 20, 2005 - 02:10 : killes(a)www.drop.org - database upgrades for the core modules with an own table - contrib modules need an upgrade too. - do we need nid and vid in both the node and the node_revisions table? - the amount of sql queries means a good stress testing for large databases. These issues are still open, btw. Especially the first one needs to be tackled. ------------------------------------------------------------------------ January 25, 2005 - 21:11 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_5.patch (51.13 KB) Here is a patch that has the database tables updated for forum, book, and page module. ------------------------------------------------------------------------ January 29, 2005 - 23:55 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_6.patch (49.18 KB) Yet another update to keep it working with head. The patch now also removes the table definitons for the page table. ------------------------------------------------------------------------ January 29, 2005 - 23:57 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_7.patch (55.69 KB) Sorry, that was the old version, this is the right one. -- View: http://drupal.org/node/7582 Edit: http://drupal.org/project/comments/add/7582

1 0

[drupal-devel] [feature] Improve functionality of block generation for book module
by Boris Mann 31 Jan '05

31 Jan '05

Project: Drupal Version: cvs Component: book.module Category: feature requests Priority: normal Assigned to: andremolnar Reported by: nysus Updated by: Boris Mann Status: patch You got me, Dries... I have since spent time testing the module on andre's site, and it does everything I can foresee needing at this point, with no functional errors. This functionality alone will make creating standalone sites that are composed of mainly static pages very, very simple. Boris Mann Previous comments: ------------------------------------------------------------------------ December 8, 2004 - 22:12 : nysus Attachment: http://drupal.org/files/issues/book_19.patch (4.85 KB) Attached is a patch for the book module that does the following: 1) Allows book blocks to appear on any page at any time, not just when a node from the book is being viewed. 2) Allows multiple book blocks to appear on the same page. This functionality is achieved by the automatic creation of individual blocks for each book when the book is created. Simply enable the book's block to enjoy the benefits of 1 & 2 above. If the blocks are not enabled, the blocks will appear only when a node from that block is being viewed (the same way it works now). ------------------------------------------------------------------------ December 9, 2004 - 04:35 : andremolnar Attachment: http://drupal.org/files/issues/book_19_1.patch (4.84 KB) +1 This is great. A good many people have asked for something like this, and I think its a nice solution. But in the end this isn't up to me. One minor error in the patch... + $result = db_query('SELECT n.title, b.block, b.nid FROM {book} b INNER JOIN {node} n on n.nid = b.nid WHERE b.parent = 0'); b.block is not a valid field in this query. this updated patch removes reference to it. andre ------------------------------------------------------------------------ December 9, 2004 - 05:54 : nysus Glad you like it and thanks for fixing that up. It was left over from an older version of my patch. ------------------------------------------------------------------------ December 9, 2004 - 05:54 : andremolnar Steve, If I apply this patch, and I attempt to configure one of the newly created blocks. I noticed that for some reason the block.module is returning "true" at line 249 (of the block module)- and is creating a form for module-specific configuration. But all that shows up on the screen is the word "array". Can you trace this back to see why - and maybe update the patch. I can continue to test your changes - anything to help this patch make it into core. andre ------------------------------------------------------------------------ December 9, 2004 - 07:29 : nysus Hmmm...probably because I tested my patch on my version of Drupal, version 4.5.1. I'm having no problems. Are you using a cvs version of Drupal to test. If so, I'll set up cvs on my site and track this down. ------------------------------------------------------------------------ December 9, 2004 - 10:53 : drumm See http://drupal.org/node/12347 for information on how the block system has been updated. When I saw that the elseif ($op == 'view') was taken out I knew immedaitely that there was something weird about this patch. ------------------------------------------------------------------------ December 9, 2004 - 10:58 : nysus OK, thanks for the tip. Sorry for the confusion. Still kind of new to making open source contributions and it's easy for me to overlook some obvious stuff like this. I'll fix this up when I get a chance. ------------------------------------------------------------------------ December 9, 2004 - 11:06 : Dries Please do, because this being a new feature, it has no chance getting committed to the DRUPAL-4-5 branch. The DRUPAL-4-5 branch is in bugfix-mode. New features go into CVS HEAD. ------------------------------------------------------------------------ December 9, 2004 - 14:32 : Dries There is quite a bit of duplicated code in the patch. Maybe it can be simplified (using a function)? Either way, it is a little weird. I haven't tried the path, and I'm not sure I understood the description. It's somewhat vague. Is the book module exporting multiple blocks that are nearly identical, yet have different display behavior? If so, why not add a simple block configuration setting to the original book block? ------------------------------------------------------------------------ December 9, 2004 - 18:51 : nysus Dries, Yes, there is some code that can be factored out and some general cleaning up that can be done. It was a little tricky to write so I left it kind of ragged around the edges until I'm sure there are no bugs. It has worked very well on 4.5.1 but I obviously need to update it to work with cvs. I'll be on that soon. As far as what it does: 1) For every new book that a user creates, a new block is associated with it. So if you create "Book A", "Book B", "Book C", you will get three new blocks visible on the block administration page called "Book A", "Book B", "Book C". The original "Book Navigation" block is still there, too. The functionality of the "Book Navigation" block is not affected at all by this patch. 2) If Book A's block is enabled, the block containing its menu will appear not only when a node within Book A is viewed, but at all times (unless the user suppresses it on certain pages with the "path" feature). When any node is that is NOT in Book A is viewed, Book A's menu still appears but it is fully collapsed. When a node that DOES belong to Book A is viewed, Book A's Book menu expands accordingly. 3) The user can also enable Book B & C's block, and have their menus appear in a block at all times as well. 4) If none of the book's blocks are enabled by the user, the module will behave just as it did without the patch. That is, when the "Book Navigation" block is enabled, the only time any book menu will appear is when a book node is being viewed. 5) It's important to note (and this was the tricky part to write) that if both the "Book Navigation" block is enabled and "Book A" is enabled, they will play nice with each other and not do nasty things like create the same book menu twice. ------------------------------------------------------------------------ December 9, 2004 - 21:51 : nysus Attachment: http://drupal.org/files/issues/book_20.patch (5.37 KB) Andre, Attached is a new patch that will resolve the problem of the block-specific stuff showing on the block configuration form. Let me know if you spot any other bugs. If it looks good to you, I'll go to work making the code look leaner and prettier. ------------------------------------------------------------------------ December 9, 2004 - 23:26 : nysus Attachment: http://drupal.org/files/issues/book_21.patch (4 KB) Dries, Andre: Here is a new and improved streamlined version of the patch. Have a look if you get a chance. ------------------------------------------------------------------------ December 10, 2004 - 00:11 : nysus Attachment: http://drupal.org/files/issues/book_22.patch (4.15 KB) Found a bug in the last version that would cause the block to jump to a different location. I think this should do it. Everything appear to work well (famous last words). ------------------------------------------------------------------------ December 10, 2004 - 06:49 : andremolnar Steve: bugs appear to be gone, and I didn't run across any other errors. This is functioning exactly as described. everyone: I personally would encourage support for this functionality. Book is a powerful navigation building tool in a site, not only with its ability to move next and back through a hierarchy of pages - but also its ability to build the appropriate navigation block without further user intervention (unlike the admin features in the menu module or taxonomy). The most frequently cited complaint about the book module is its inflexibility when it comes to when and where the block shows up. I also frequently hear requests for the ability to show multiple book blocks at the same time. Up until now the best alternatives suggested required users to do a hack (e.g. build a custom block that calls such and such a hook). Most abandon their request at that point because its over their heads. With this patch all those requests are covered and more. Now all books can automatically have their own block and admins can easily decide when and where each of those individual blocks show up (left right, up down) and coupled with the new configuration features of the block module - its very easy for admins to decide on which individual pages a block will show up. I would be interested what other have to say about this feature. My only reservation (which is minor compared to the benefits of the functionality offered) is that there is no way to turn this functionality off. i.e. The default behaviour is to build individual blocks for each book. If there could be a way to toggle this feature on and off somewhere - it would be perfect. Still, AS IS - this is a major improvement and offers great flexibility to admins and site creators. andre ------------------------------------------------------------------------ December 10, 2004 - 07:04 : Anonymous Andre, Thanks for the feedback on the usefulness of this module. Glad I could pitch in and help. I agree about the inability to turn the feature on/off and I was thinking about that myself. I think it could easily be accomplished by creating a checkbox in the "book navigation" block individual configuration's settings. Call it "Enable individual book blocks." When enabled, the individual book blocks will appear on the block administration menu. One question: Where would the state of this checkbox get saved? Has Drupal moved away from serializing data in the data base? ------------------------------------------------------------------------ December 10, 2004 - 10:36 : Dries I'm afraid that 'Enable individual book blocks.' is not descriptive/clear at all. Are you suggesting a setting to toggle between 'show block on all pages' and 'show block on book pages only'? ------------------------------------------------------------------------ December 10, 2004 - 11:45 : nysus Dries, No. Andre and I suggest a setting within the "Book Navigation" cofiguration page, that would toggle whether or not individual books appear on the list of all blocks on the block administration page. Hence the name 'Enable individual book navigation blocks.' The help text for this checkbox might read something like: "By default, a book's navigation block is visible only when a page from that book is being viewed. Check this box if you want more control over where and when an book's navigation block is visible. You will then be able to control the book's navigation block location and visibility settings on the "admin/block" page." Hope this makes it pretty clear. ------------------------------------------------------------------------ December 10, 2004 - 12:08 : Dries I understand what you are trying to do, but not how you are trying to do it, or how the setting is supposed to work. I guess I'll have to try it when a new patch lands. ------------------------------------------------------------------------ December 11, 2004 - 01:52 : nysus Attachment: http://drupal.org/files/issues/book_23.patch (5.02 KB) Alright, fellas, I'm proud to unveil my crowning achievement in the open source development world (no big deal to most of you guys but pretty good for a hack like me). Thanks for all the input so far. It's been helpful. I've streamlined the heck out of it per Dries suggestion and I've created an option to turn this functionality on an off per Andre's suggestion. Does this look good to you guys? Anything else I have to fix or improve? Thanks. ------------------------------------------------------------------------ December 11, 2004 - 05:02 : Dries I tried the patch. It works great but to me, the 'Give books their own block' settings seems to be redundant. Why not export the current book navigation block, along with an additional block for each book? Looks a lot simpler to me. I think I spotted a bug: orphaned book pages (or possibly book pages that are unpublished) appear to be getting book navigation blocks. ------------------------------------------------------------------------ December 11, 2004 - 09:34 : nysus I'll see if I can fix the bug. Might be tricky. But I don't understand your recommendation to "export the current book navigation block, along with an additional block for each book". Can you expand on this thought? ------------------------------------------------------------------------ December 11, 2004 - 10:06 : nysus Dries, I am unable to duplicate the bug. I have three orphaned pages. I also tried unpublishing some pages. But as far as I can tell, the patch works as expected. ------------------------------------------------------------------------ December 11, 2004 - 11:00 : Dries If you can't reproduce the problem, chances are my node/book table is somewhat fubar. As for the configuration option. I suggest removing it and to always make these new blocks available on the /admin/block configuration screen. ------------------------------------------------------------------------ December 11, 2004 - 11:52 : moshe weitzman I am hoping that we maintain the option to keep the behavior where the appropriate book block only shows up when its book page viewed. this is a nice, tidy arrangement. ------------------------------------------------------------------------ December 11, 2004 - 12:03 : nysus Yes, if you don't enable any of the individual book blocks, the a book's block (i.e. navigation menu) will only appear when a page in a book is viewed. In other words, you have the option to have the book block act like this patch isn't even installed. ------------------------------------------------------------------------ December 11, 2004 - 12:54 : Dries I guess I'll have to try the patch again, because I don't understand why it works like this -- or at least, why it can't be made simpler. ------------------------------------------------------------------------ December 11, 2004 - 13:13 : nysus Can you be more specific? Why it works like what? ------------------------------------------------------------------------ December 11, 2004 - 13:24 : nysus Attachment: http://drupal.org/files/issues/book_24.patch (4.1 KB) This patch reverses a change made in the last patch which required a user to enable individual book block before they could enable any individual book blocks. ------------------------------------------------------------------------ December 11, 2004 - 13:41 : andremolnar As I mentioned earlier I am *fine* with either version of this patch as long as it makes it to core. But, as I said earlier, I clearly think the preference for admins would be to have the option to enable or disable this functionality. BTW - if this does make it in, I would be happy to create a Handbook page that describes the new features - something like "how to build robust site navigation using the book module". (on or after December 17th). andre ------------------------------------------------------------------------ December 12, 2004 - 04:11 : Anonymous I am not at all happy with these features. They are incosistant, confusing and should use exising methods and UIs. May main concern is the incosistancy: it is confusing, will require extra attention with each core code change, adds extra logic to the core, and is not re-useable. so here are my questions: 1) why do we not use the menu system and apis to build and adminster the trees? Saves code, does not add extra UIs, and gives users more power. 2) why do we need that extra showing logic? a book block should not get exeptional if clauses, it should use the existing path setting methods on block admin. extra logic is confusing for administrators (hey, i set the path so that the book-block should show up here, but it does not, why?) we should really not provide extra logic in the block hook, but should rather use default settings in block admin (the book could fill the bookblock sql cells with custom paths, for example) 3) we should avoid extra UIs. We already have far too much, and far too much different ones. Please rather improve the block admin, than add new separate interfaces. 4) why do book blocks need al these expeptions in the first place? If they are so exeptional, we could consider not using blocks, but something else, like in-line book layout (pages with the index etc) 5) why did you not choose for a general, standard, block gerneation API? that way modules, such as taxonomy, image gallery, weblinks, article, etc can reuse it and introduce block gernation. All that sayd, i like the idea of this functionality, but i fear for a great useability downfall if we start introducing all sorts of exeptions for all sorts of modules. Because now chances are very big that taxonomy, image gallery etc will need to introduce other UIs, other code, new methods and new documentation, if they too want some sort of better block handling. so a -1 from me. ------------------------------------------------------------------------ December 12, 2004 - 04:12 : Bèr Kessels ^^--- That was me (bèr kessels) forgot to log in..... ------------------------------------------------------------------------ December 12, 2004 - 04:34 : nysus I'm not going to pretend I can argue if my patch does or does not fit well into Drupal's larger architecture. My motivation for writing it is that I had an immediate need to create an easy way to make it easy for users to create menus. I'll let others decide whether or not the patch has merit from the big picture perspective. But if it doesn't, why not just use it for its immediate benefits and then throw it away when something better comes along? ------------------------------------------------------------------------ December 12, 2004 - 05:01 : Dries My summary is this: +1 on the functionality, -1 on the implementation. The code itself is good, but the usability/integration is not. ------------------------------------------------------------------------ December 12, 2004 - 05:13 : nysus When you say "usability" is that from the user's perspective or the code maintainers? I'm guessing it's the latter but I'm unsure. What about the idea of using the patch until a more permanent solution comes along? Yes, it's much better to live in a home with indoor plumbing but why not use the outhouse while you wait for a toilet to get installed? Or are there other considerations I'm overlooking that would make this a bad idea? ------------------------------------------------------------------------ December 12, 2004 - 05:20 : Goba nysus it is just generally against the Drupal philosophy to add improperly implemented functionality until something better comes along. There even used to be ocassions in Drupal releases, when some functionality was removed (not fixed) for a release, because its implementation was not adequate. ------------------------------------------------------------------------ December 12, 2004 - 05:55 : Dries Usability for the user. The extra setting on the block configuration page is both confusing and awkward. I don't understand why things must be configured/enabled that way (see my and Ber's previous comments on this issue). ------------------------------------------------------------------------ December 12, 2004 - 06:03 : nysus Well, just for the record, I reversed that functionality per your suggestion and uploaded the patch. The indiviudal book blocks now appear automatically. ------------------------------------------------------------------------ December 13, 2004 - 05:56 : Bèr Kessels Hi, I am sure you can make not only simpler, but better usefull for admins and users. All you need to do is use the menus for this. i.e. make a menu entry for each book page. For each book make a menu on level 0, without a parent. that way they become a seoprate menu, each with an own block. it saves code, makes things more consistent, and most important, uses drupal functionality where it should. ------------------------------------------------------------------------ December 13, 2004 - 06:50 : nysus Ber, Are you suggesting that for every single book page that a menu item be created? That's really not practical. That was my main motivation for writing this patch: to make it easy to put links, not necessarily related to one another, into a block. Any more pages than 10 and the sheer tedium of the job would prevent anyone from ever doing that. The menu.module is great, but adding new menu items is far from quick and painless. I just added about 10 to my menu for different taxonomies on my site and it wasn't fun. Plus, if you do as you suggest, there is also the problem of the book showing up twice. It will be generated by the menu and then it will be generated again by the book module which is programmed to design a block. You'd have to put some logic in the book.module _block hook to try to anticipate if a user has enabled a book in the menu. That wouldn't be pretty code. I'm all for putting automatic generation of book navigation blocks as part of the menu module. It does make more sense to have it there. But it forces me to ask the question: "Then why do we currently have code in the book module that generates a menu? Shouldn't that belong in the menu.module, too?" ------------------------------------------------------------------------ December 13, 2004 - 14:31 : killes(a)www.drop.org I think what Ber is trying to say is that you can write a contrib module that monitors the changes to the book table and creates menu items automatically. nodeapi is your friend. I would also prefer this solution. ------------------------------------------------------------------------ December 15, 2004 - 08:39 : Anonymous ""Then why do we currently have code in the book module that generates a menu? Shouldn't that belong in the menu.module, too?" " Because it's old code. It would be nice if book.module generated this block using its _menu hook, so that the admin would have a few options in terms of configuration. "Plus, if you do as you suggest, there is also the problem of the book showing up twice. It will be generated by the menu and then it will be generated again by the book module which is programmed to design a block. " No. The old code which manually builds a block in book.module would be removed. Book blocks would only be generated by the menu. This would also have the added benefit of allow the administrator to easily place a book in an appropriate spot in the menu tree, while still allowing the possibility of displaying it in a separate block. Because of menu caching, I don't expect a large performance hit for creating the menu items. "That was my main motivation for writing this patch: to make it easy to put links, not necessarily related to one another, into a block. " It sounds like instead of (mis)using book.module, your time would be better spent in a usability improvement to menu.module so it's easier to do this. ------------------------------------------------------------------------ December 15, 2004 - 22:55 : nysus Thanks for the feedback and input. I appreciate it. However, I would also appreciate if you took more care to avoid the condescending tone in your post: "It sounds like instead of (mis)using book.module, your time would be better spent in a usability improvement to menu.module so it's easier to do this. " It's really quite unnecessary and off-putting. Though it won't stop me from contributing to Drupal in the future, I'm sure others would be really turned off by such a patronizing comment and it could dissuade them. I'd like the Drupal community to be a welcoming and friendly place that will inspire people to contribute, not discourage them. Thanks. ------------------------------------------------------------------------ December 16, 2004 - 02:07 : Bèr Kessels nyesus, Please do not start /that/ discussion here. :) Drupal community is known fo being direct, maybe because of the big number of western-Europeans attending, maybe because of other reasons. No-one commented that you are wasting your time. But the commentor was telling you somthing likethis: "If you would follow the previous sugeestions, your added feature would be much better appreciated, and will probably work much better for you too". He/she was by no means telling you to stop your silly coding, or anything in that line. He/She only wanted to show you the obvious and better direction. We often deal with issues that add some feature, and a complete new UI, because the author does not like, or cannot use the existing UIs and features. This is nogt good, because if that same author would have spend his/her time on improving that existing functionality or UI (improving is not neccesarily the same as extending!!) that code and time would benefit all much better. Thats what the commentor tried to say. And so is it here: If you dislike the way the books handle the blocks, and if you do not want to use the menu, because you do not like its UI, then do not add another UI and more features, but rather merge these, and improve the parts (in the menu) you dislike. ------------------------------------------------------------------------ December 16, 2004 - 02:35 : Dries We can worry about the menu integration later. Let's focus on the new option's usability/interaction design first. ------------------------------------------------------------------------ December 16, 2004 - 06:51 : nysus I understand what the commenter was saying and like I said, I appreciate and understand it. I'm not upset and I'm not looking for an argument, I was just being direct as well. :) As part of the Drupal community (albeit a minor player), all I'm saying is that I would like to see folks not have a tin ear to the humanity in all of us. It will help make Drupal an even stronger community and attract even more talented developers. Human interaction is part of the development process. Whether we like it or not, we must cope and deal with it. ------------------------------------------------------------------------ December 18, 2004 - 01:54 : Dries I thought some more about this and am starting to believe that integration with the menu system should take priority. Here are common cool scenario's: I want to create a separate navigation block for the 'Drupal handbook'. I want to add a menu item called 'handbook' to the user block. That is, I want the book navigation to be part of the existing user navigation block. I want to add a menu item called 'handbook' to the top-level navigation menu. How would that work from a user's point of view? What do I have to click and where to configure this? ------------------------------------------------------------------------ December 21, 2004 - 10:47 : andremolnar I was actually thinking about the same thing last night (must have been something in the arctic air). / 2. I want to add a menu item called 'handbook' to the user block. That is, I want the book navigation to be part of the existing user navigation block. 3. I want to add a menu item called 'handbook' to the top-level navigation menu. / This is already possible (to a certain extent) with the current Book.module and Menu.module - A 'Books' menu item is created in the user navigation by having the Book.module enabled. Menu.module allows you to enable/disable this menu item. Menu.module also allows you to re-name this menu item. But, this only helps if you only intend to have 1 book (or else the name 'Handbook' is misleading if the user finds more than 1 book listed). - so this isn't good enough (or is it). /1. I want to create a separate navigation block for the 'Drupal handbook'./ This is what I was trying to figure out. Not just this, but a different way to do what Nysus was attempting. i.e. create a menu block for each and every book that is created. There is a way to write code that would (re)build a 'custom menu' on every add/edit/update to a book page - or book outline update. Custom menu's automatically have a block created for them. But, this I think would be a misleading use of the 'custom' menu - as the menu would not be custom if they are a part of core. So, I would think that two new constants could be added to the menu.inc file - MENU_BOOK_MENU - and MENU_BOOK_ITEM - each would behave as custom menu's, but would be reserved for books. These menu types should NOT show up in the Menu.module administration - because the administration of the book_menu items would be done by administering the book itself (adding an item, removing an item, moving an item up/down in the hierarchy, assigning parents etc.). However, the blocks that the book_menus would create would show up in the block administration (so users could enable/disable each block - and decide where on the site they show up). The existing book block logic would be removed. So the logic would be: If a creates a book in the book administration page - the Book.module automatically creates a new MENU_BOOK_MENU Any time a user adds to or updates or delets a book page - the entire book menu is deleted and recreated based on the hierarchy defined by the book itself. The blocks for each book would show up in block administration. Any thoughts - I know that this doesn't exactly address points 1 and 2 - but it could act as a first step. Is this approach a bad idea? It would add special conditions for the proposed book menu's - but books would be a special case. Even if people don't like this solution, maybe it will give someone a better idea. I'd love to hear them. andre ------------------------------------------------------------------------ December 21, 2004 - 14:55 : Boris Mann +1 to this andre I had promised to put down my thoughts on this matter, as it relates to 1) primary and secondary links and how they are managed and 2) auto-generation of primary/secondary navigation based on book outlines So, for 1), we currently have functional-but-not-very-usable plain textfields to manage primary and secondary links. I would like to see menu.module used to control all navigation links, whether it is the navigation block or primary/secondary links. What is needed: a) default system menus labelled "primary" and "secondary" which would store; this is where modules could insert navigation b) support for full URLs (e.g. http://myexternaldomain.com) instead of just path 2) if we got 1 working, and andre does his book menus, this could get taken care of automatically. Basically, for brochureware/business/etc. sites that have static content, you could have a root book as one of the primary navigation links, and then the secondary links are generated automatically. ------------------------------------------------------------------------ December 22, 2004 - 13:02 : Dries I agree with Andre that the book module's integration with the menu system needs to be worked on. I support any effort that makes it easier to structure pages and that makes it easy to link pages/nodes from within a menu. However, I'm opposed to putting book module specific names in the menu system (eg. MENU_BOOK_MENU and MENU_BOOK_ITEM). I can imagine a handful of modules that want to maintain a menu tree (or part thereof) so I'm in favor of generic names. I'd have to read up on the menu system code, but last time I checked there was a MENU_MODIFIABLE_BY_ADMIN flag. You could choose not to set this flag for the menu items generated by the book module. Maybe it's already possible to implement to implement Andre's suggestion without having to modify/extend the menu system. Are you exploring this path? ------------------------------------------------------------------------ December 24, 2004 - 02:01 : andremolnar I've had a few spare hours to work on this. I've started to cobble together a solution - but in doing so I discovered a bug in the menu module (for which I will submit a seperate issue - if one doesn't already exist). The principal I suggested works. I added some code to the book module that creates custom menu's (MENU_CUSTOM_MENU with MENU_CUSTOM_ITEMs) for each Book that exists in a site. This is just as a proof of concept - I chose this menu type to start with because they automatically have a block created for them in admin/block (which is ultimatly the functionality we want). I ran a test and the menus are created as expected - the blocks are also created. But when I tested the menu blocks by enabling them I ran into a problem. It seems as though the menu system does not expand/explode sub menu items if the node type is book. I'm not sure why this is, and I haven't traced the source code yet - I thought I would ask if anyone has intimate knowledge of the menu system if they can point me in the right direction. No patch attached because until that problem is fixed this proposed change won't fly :( andre ------------------------------------------------------------------------ December 24, 2004 - 03:11 : Dries Just a wild guess: maybe it doesn't work because the book module's URI scheme is not hierarchical? ------------------------------------------------------------------------ January 7, 2005 - 02:45 : andremolnar Attachment: http://drupal.org/files/issues/book_module_1.patch (5.1 KB) I finally took some time out to do some work on this. As mentioned in a post to dev the problem I was having with menus not expanding/contracting properly was some crud in my database. Once that was cleared up my changes worked fine. I am attaching a patch for comments and for the brave willing to give it a test drive. History: This thread and http://drupal.org/node/15198 and http://drupal.org/node/15153 node/15198 has a patch that is required for this patch to work. What this does: Pretty straight forward. Any time a user adds/updates/deletes book or book pages (including via outline) - a function is called to create a new menu for each book. Any existing book menus are wiped out and then the new book menus are inserted - and the system menu is rebuilt to reflect the changes. The menus created consist of type MENU_MODULE_MENU and MENU_MODULE_ITEM. These menus show up in the menu/admin page so that admins can be aware of them, but the menu types are not editable via menu/admin (all changes are handled by the book module). Since the menus are in the menu table, the menu_block() hook handles the creation of the blocks for each of them. The blocks can then be administered in the usual ways via the block/admin interface. KNOWN ISSUE: (suggested solutions welcome) Since the menu table is updated on every change to books - the blocks associated with the menus are also recreated with default settings (i.e. disabled, and with no path or throttle settings) requiring a user to take an extra step and re-configure the book blocks for viewing on their site. I think this is unacceptable. For the casual user of the book module that only has a single book that doesn't change often, this would not be a big deal. But, if anyone wanted to make use of book module to handle dynamic site navigation this would create more work than it saves. Looking forward: If the block generation issue can be solved in a tidy way, this patch could allow users to use book to handle all their site navigation generation needs. Also, this patch could allow for the removal of a large chunk of code in the book module dedicated to building its own block via the block system. I left it there for now because I suppose there may be those out there that want to have book.module work the way book.module always worked (only show the book block when viewing a book page). Even so, since each book would have its own block, an admin could specify when and where the block shows via admin/block. I would appreciate feedback. If nothing else I hope this gives someone some new ideas. I'll continue to work on the block regeneration issue. andre ------------------------------------------------------------------------ January 7, 2005 - 02:47 : andremolnar Attachment: http://drupal.org/files/issues/book_module_2.patch (4.98 KB) sorry - here's a patch with prefered line breaks. andre ------------------------------------------------------------------------ January 7, 2005 - 08:47 : nysus Andre, I've been meaning to give this a throrough look when I get a chance. Hopefully this weekend. ---Steve ------------------------------------------------------------------------ January 19, 2005 - 18:02 : andremolnar Attachment: http://drupal.org/files/issues/book_module_3.patch (6.66 KB) Here is an updated patch. Same comments as followup 51 - except that the known issue has been resolved. This changes book module so that any action taken on a book, including adding new books or book pages will create a menu in the menu system for that book - and thus create blocks for those menus that can be administered in the usual ways. This is my first attempt at a major patch to core - comments are welcome I will be happy to update documentation once revisions to the code have been taken care of. andre ------------------------------------------------------------------------ January 19, 2005 - 18:53 : andremolnar Attachment: http://drupal.org/files/issues/book_module_4.patch (6.17 KB) Sorry - would be nice if I removed some debugging code - ;-) also previous patch also would have introduced a need for a change to the menu table that isn't required yet. This patch is a correct version andre ------------------------------------------------------------------------ January 20, 2005 - 11:16 : Dries The menu is recreated every time a book page is updated. I believe this is unwanted behavior because it requires the book block to be reconfigured upon every update. ------------------------------------------------------------------------ January 20, 2005 - 11:32 : andremolnar The menu is indeed re-created with every book page edit - because if the book page title changes the menu needs to reflect this change. The book block re-configuration is not required by the user. The code stores this information and re-sets the block settings. I will see if I can test for 'title change' before forcing the re-build of the menu - It would save a bit of processing power. andre ------------------------------------------------------------------------ January 20, 2005 - 12:21 : andremolnar Took another at this - and it turns out that I already have a check to see if title or weight change (rather - they already existed in the module and I used them). andre ------------------------------------------------------------------------ January 23, 2005 - 18:22 : andremolnar Attachment: http://drupal.org/files/issues/book_module_6.patch (7.42 KB) Feedback received indicated that the original block generation code in this module should be removed - since this patch hands block generation off to the menu and block system. This patch removes that code. BUT - It should be noted, that in order to have book blocks only show up on pages of node type book - an additional patch found at http://drupal.org/node/16074 is required. andre ------------------------------------------------------------------------ January 24, 2005 - 10:36 : andremolnar Attachment: http://drupal.org/files/issues/book_module_7.patch (7.43 KB) Yet another patch: Brings this patch in line with - http://drupal.org/node/16074 (i.e. the column name change in {block} table) andre ------------------------------------------------------------------------ January 25, 2005 - 10:56 : andremolnar Attachment: http://drupal.org/files/issues/book_module_8.patch (7.53 KB) And another minor patch to cover coding style and variable names. Just to bring everyone up to speed: 1) Each book and its children pages have a menu created for them (on any add, edit, delete to a book or book page). 2) Those book menus automatically have a block created for them via menu_block hook. 3) Block settings are preserved any time there is a change to a book that requires the menu and its associated block to be re-built. 4) book_block hook is removed because it is redundant. 5) With the addition of the configuration option in blocks to only show blocks on certain node types (see: http://drupal.org/node/16074) administrators have full control over when and where a book block shows up (including maintaining the status quo of only having book blocks show up on book pages). 6) This patch does require http://drupal.org/node/15198 andre ------------------------------------------------------------------------ January 25, 2005 - 13:49 : Boris Mann +1!! Book-based automatic navigation for corporate/brochureware sites! Fantastic! Chris Messina, get in here and add a +1 to this. ------------------------------------------------------------------------ January 25, 2005 - 14:26 : Dries Boris, have you tested this patch extensively, or are you just giving this +1 based on what you've read? If you want to see this committed, take the time to review/test it. ------------------------------------------------------------------------ January 25, 2005 - 21:45 : andremolnar Yes - please - testers are welcome. Nysus (aka Steve) has already agreed to give this a test drive when he had a spare moment. I hope for some feedback in the near future. But if you have a moment Borris I would appreciate it. OR - If someone would just like to demo this: http://s92258948.onlinehome.us/greenbeach/ UN: drupaldev PW: drupaldev Don't mind the mess - it is my test environment and there is a lot of junk data and settings floating around - but this patch is running there along with the additional block configuration settings. andre -- View: http://drupal.org/node/14120 Edit: http://drupal.org/project/comments/add/14120

1 0

[drupal-devel] [feature] blogapi--Allow posting to multiple node types
by walkah 31 Jan '05

31 Jan '05

Project: Drupal Version: cvs Component: blogapi.module Category: feature requests Priority: normal Assigned to: walkah Reported by: nedjo Updated by: walkah Status: patch Attachment: http://drupal.org/files/issues/blogapi-multiple-types2.diff (7.19 KB) here's an updated patch that allows administers to control which node types can be posted via blogapi (as per nedjo's suggestion). also i've added some documentation about the 'unusual' usage. please apply for 4.6 :) walkah Previous comments: ------------------------------------------------------------------------ March 3, 2004 - 15:43 : nedjo Attachment: http://drupal.org/files/issues/0202.HEAD.nedjo.blogapi_module.patch (2.35 KB) The attached patch adds a setting to the blogapi allowing site admins to choose the node type that blogapi entries will go to. This will be useful when wishing to do regular story (as opposed to blog-type) entry posting through the api. questions to Nedjo Rogers nedjo(a)gworks.ca ------------------------------------------------------------------------ March 17, 2004 - 22:47 : moshe weitzman I'd like this functionality too. But this patch may cause moe harm than good. If user chooses story or blog, the patch works. If he chooses book, or recipe or poll, etc., he will get a validation error. Thes node types require more information. ------------------------------------------------------------------------ March 22, 2004 - 15:07 : nedjo Attachment: http://drupal.org/files/issues/0202.HEAD.nedjo.blogapi_module_0.patch (2.32 KB) Thanks for noting this; I'd failed to test with other node types. I've modified the patch to limit the choice to either "story" or "blog". ------------------------------------------------------------------------ August 12, 2004 - 10:20 : killes(a)www.drop.org Doesn't apply anymore. ------------------------------------------------------------------------ August 14, 2004 - 22:04 : nedjo Attachment: http://drupal.org/files/issues/blogapi.allow-choice-of-node-type.patch (3.75 KB) Here is an updated patch. Now enables three choices: blog, page, and story. ------------------------------------------------------------------------ August 21, 2004 - 12:12 : Dries I'm not going to commit this unless there is more demand for it. ------------------------------------------------------------------------ August 21, 2004 - 13:56 : nedjo This functionality has been requested here [1] (/Posts made by w.Bloggar seem to be an entry other than "Story," which is what I want. I don't see any way to make w.Blogger post to this node type./) and here [2] (/they automatically post "personal blog entry" node types, I'd want them to post "story" nodes/). My judgement is that blogapi could readily be used considerably beyond the limited blog application. This small change would make the highly developed blogapi into a tool for general site posting rather than restricting it to what is, for many sites, a little-used medium, the blog. [1] http://drupal.org/node/view/8055 [2] http://drupal.org/node/view/9852 ------------------------------------------------------------------------ October 18, 2004 - 01:15 : Anonymous Here's another vote for extending the blogapi to allow posting stories and other node types! This would be a great addition. ------------------------------------------------------------------------ October 18, 2004 - 06:46 : Robert Castelo This would be really useful for Drupal sites I've set up for companies - where employees enter content into a taxonomy created by me and the (client) project manager. Most employees will be able to use a point and click desktop application much better than a Webpage interface with clunky Javascript controls (none of them know HTML). Extending the blogapi to other node types would add considerable value to Drupal for business use. ------------------------------------------------------------------------ October 18, 2004 - 07:16 : moshe weitzman +1 for the functionality. but there is a problem with the patch. it hard codes permission names from 3 different modules. instead, use node_access('create', $type). that fragment was taken from the end of node_add() ------------------------------------------------------------------------ November 26, 2004 - 10:48 : teradome An idea: Since blogid is a string identifier according to the APIs, why not allow node types to be selected by using the name of the node type itself? You could keep the checkbox list of available nodes in the blogapi settings page (blog option on by default), which would also list what blogid string you need to enter in your remote client to access it. This way you can add them in incrementally. Legacy support via the user's system # could be maintained, but "blog" as a blogid would become the preferred way of "editing own blog." (Obviously, you can't do more than your own, given the simplicity of the APIs.) Even more hardcore would be to implement this into a hook, so other modules could add blogapi support (I'm thinking "quotes" for one). Since you would identify by the node type, there's no overlap. ------------------------------------------------------------------------ November 26, 2004 - 11:06 : walkah teradome : excellent idea! i like this approach... and it should work well within the (limited) framework of the apis... the only thing is that several editors present this as a list of your "blogs" which may make it a bit confusing. however, it would cram this much desired functionality into the existing framework. let me play with it for a bit, and i'll prepare a patch. ------------------------------------------------------------------------ November 26, 2004 - 11:41 : garym(a)teledyn.com +1 from me on this one, although my own needs need a more generic REST/webservices interface way beyond bloggerAPI. I'd love to see micro-content publishing gateways such that we could seamlessly post complex items to the site, for example, posting member calendar information directly from a cellphone into the events.module or a Firefox XPI that exports/merges personal bookmarks files into the weblinks. ------------------------------------------------------------------------ November 26, 2004 - 12:11 : FactoryJoe(a)civicspacelabs.org +1 on this patch (or the functionality at least). Right now, MarsEdit [3] can't use Drupal categories because of limitations in the XML-RPC API (according to brent @ ranchero.com) It sounds like this kind of general change would enable me to use MarsEdit (or any other blog editor )to its fullest potentional. Oh, and excuse any technical errors with my assessment... I only half-way know what I'm talking about. :1 Chris [3] http://ranchero.com/marsedit/ ------------------------------------------------------------------------ November 26, 2004 - 15:07 : mike3k I really want to see this feature. I have two story based Drupal sites and I'd like to be able to post from MarsEdit. I like the suggestion of using 'story' or 'node' as blog ID. ------------------------------------------------------------------------ November 26, 2004 - 22:25 : nedjo Attachment: http://drupal.org/files/issues/blogapi-allow-choice-of-node-type.patch (3.55 KB) Here is an updated patch partially addressing Moshe's suggestion to remove hard-coded module references. I've left in hard-coded references in the "edit own..." instance because the word needed is different from the module name (e.g., "stories" for the story module). "why not allow node types to be selected by using the name of the node type itself? " I don't fully understand the suggestion of using the node type as an id. The id is set (I'm thinking) by the server--but the client has to first make the posting. How would a client select which type to create? "Right now, MarsEdit can't use Drupal categories because of limitations in the XML-RPC API (according to brent @ ranchero.com) It sounds like this kind of general change would enable me to use MarsEdit (or any other blog editor )to its fullest potentional. " This patch won't I think do anything to address that issue. The new release candidate of w.bloggar also seems to fail to set categories (when used in Movable Type mode). Likely we need to update the handling of category requests. ------------------------------------------------------------------------ November 29, 2004 - 09:16 : Anonymous I'm a MarsEdit user and would love to use it with Drupal. The XML/RPC features need to have more visibility, there are many who *would* use them if they would know they exist and how they work. ------------------------------------------------------------------------ November 29, 2004 - 15:01 : walkah Attachment: http://drupal.org/files/issues/blogapi-multiple-types.diff (3.99 KB) here's a patch that implements multiple node types using the (admittedly very clever) blogid hack. basically, you will appear to have "separate blogs" for each node type that you can post to. (yes moshe, posting will fail for recipies and events, etc) you can maintain different taxonomy vocabularies for each node type - which will work. For those having trouble with MarsEdit : try setting the "Software" to movabletype... it works for me with 1.0b12 . Brent has an old profile for drupal in there it seems - back when we only did blogger api. Please give this patch a try and offer feedback. thanks :) ------------------------------------------------------------------------ November 29, 2004 - 19:26 : nedjo Thanks for the patch. I am mystified by it, but this may just reflect my ignorance. Could you explain how to use this from the client end (e.g., using w.bloggar). How/where does one indicate that a particular post is e.g. a "story"? Does this patch accomplish the primary desired aim of this issue--enabling the use of blogapi by sites that don't use blogs? ------------------------------------------------------------------------ November 30, 2004 - 08:48 : walkah nedjo: essentially this patch follows teradome's suggestion to make use of the formerly unused 'blogid' parameter that exists as part of the APIs - specifically using that parameter to specify the node type to create. how *exactly* this is used on the client side is dependent on how the client handles the fact that the API allows for multiple blogs per user account. i don't have access to a windows machine at the moment, so I can't tell you specifically for w.bloggar, but i'll give you two examples from the mac world: attached to this issue is a screenshot of ecto (http://ecto.kung-foo.tv/) which is my preferred app. as you can see on my "local test" account i have 3 "blogs" - blog,page and story. thus posting to the "story" blog creates a new story. marsedit (http://ranchero.com/marsedit) does not automatically list all blogs for a user account, rather you must specify a single blog id for each account. so, you'd enter "blog" (for example)... where as that previously defaulted to your userid (and was unused). does that make sense? ------------------------------------------------------------------------ November 30, 2004 - 11:24 : walkah Attachment: http://drupal.org/files/issues/ecto-blog-types.jpg (48.3 KB) screenshot of how this patch looks in ecto attached for real this time :P ------------------------------------------------------------------------ November 30, 2004 - 12:06 : nedjo Thanks for the explanation, I get it now. This approach has the clear advantage (over what I'd suggested) of enabling a user to select from various node types rather than a single server-designated one. But it depends on settings on the client. This means extra instructions to users, instructions that will vary by the client. More importantly, some clients won't support this functionality. For example, I don't find a way to specify multiple blogs per user with w.bloggar (a commonly used client for Windows users). So, while this is in my view definitely an improvement on the existing module, it doesn't fully meet the identified need. I would therefore like to see this patch in combination with the changes I suggested. I.e., user can select which node type to post to (as per walkah patch), admin can set default type if no type designated by user (my patch). That way there is advanced functionality for clients that support it and a default solution for clients that don't. Also, likely we should have an admin setting to limit the list generated by function _blogapi_get_node_types() to designated types. I hard-coded this to "blog", "page", and "story", but we could instead have a multiple select. Thoughts? ------------------------------------------------------------------------ November 30, 2004 - 13:32 : Boris Mann If you can't specify multiple blogs per user, the client is broken. Seriously -- it means it won't work with Blogger or Blogware, to name two I know of right of the bat that support multiple blogs per user. Reading through w.bloggar's site, it does support multiple blogs for Blogger. If someone on Windows could verify that it also works with James' patch, I think we're done. As walkah recently posted on his blog, we need to try and get better APIs, rather than continually kloodging on both the client and server side because of the limitation of the API. ------------------------------------------------------------------------ November 30, 2004 - 14:04 : nedjo Okay, I agree, this fully meets the need, apologies for my slowness, it's just taking me a bit to catch on. The only change needed to the patch I believe is to edit _blogapi_get_node_types() so that it will generate a limited list of node types (only those that blogapi will work with) so that users won't get errors. This could be hard-coded to "blog", "page", and "story" or could be a multi-select (a new setting) set by site admins. E.g., to blogapi_settings, add something like (I haven't tested this): <?php foreach (node_list() as $type) { $node_types[$type] = node_invoke($type, 'node_name'); if (in_array($type, array('blog', 'page', 'story')) { $defaults[] = $type; } } $output .= form_select(t('Blog types'), "blogapi_node_types", variable_get('blogapi_node_types', $defaults), $node_types, t('Select the content types for which you wish to enable blogging.'), 0, 1); ?> and change _blogapi_get_node_types() to: <?php function _blogapi_get_node_types() { global $user; $approved_types = variable_get('blogapi_node_types', array('blog', 'page', 'story')); $types = array(); foreach (node_list() as $type) { if (node_access('create', $type) && in_array($type, $approved_types)) { $types[] = $type; } } return $types; } ?> ------------------------------------------------------------------------ November 30, 2004 - 14:56 : Dries ... and document this peculiar usage in the help/documenation? ------------------------------------------------------------------------ January 13, 2005 - 12:40 : teradome man, i can't believe i'm just seeing this patch now... the tracker really needs an option to follow "issues" that you've replied to! ------------------------------------------------------------------------ January 13, 2005 - 12:59 : teradome This. is. sweet. And just in time for my new workflow changes (blog=no comments, forum=comments desired)! Thank you walkah! -- View: http://drupal.org/node/6195 Edit: http://drupal.org/project/comments/add/6195

1 0

[drupal-devel] [bug] File attachments are sometimes saved with an incorrect mimetype
by tangent 31 Jan '05

31 Jan '05

-Project: Project +Project: Drupal -Version: cvs +Version: 4.5.2 -Component: Issues +Component: file system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: tangent Updated by: tangent -Status: active +Status: patch Attachment: http://drupal.org/files/issues/file_mimetype.patch (3.81 KB) This issue is a symptom caused by faulty mimetype handling in file.inc so I'm updating the issue to the correct component. The old title was "File attachments with '.inc' in filename are served as HTML". Apparently, the real issue is that file.inc currently stores the content-type presented by the user-agent when file is uploaded. This is not the best thing to do because a user-agent can present an incorrect content-type either inadvertantly or, when used by a malicious person, purposefully. There are a couple of methods to determine the actual mimetype of a file but they all have drawbacks. The Fileinfo (http://pecl.php.net/package/fileinfo) PHP extension is the recommended tool for the job but is not typically available on a typical host. The mime_content_type() function requires the "file" program to be available which is not always the case either. Then there is the fallback method of testing the filename extension against a list of internally known extensions. I've created a patch which attempts the first 2 options and then falls back to the third. There may be a better option which I have not thought of though so feel free to offer suggestions. tangent Previous comments: ------------------------------------------------------------------------ January 24, 2005 - 23:29 : tangent Files attached to issues (like patch files) which have "inc" in the filename are incorrectly served with a text/html mime type and their contents are marked up with html. This is clearly inappropriate. See the issue below for some examples of this occurance. http://drupal.org/node/16021 ------------------------------------------------------------------------ January 24, 2005 - 23:51 : tangent A filename with "module" in the name is also served the same way. Perhaps we should also test for "mysql, pgsql, php, css, xtmpl, sh, pl, txt" and any other text file extensions. If this is not desirable, at least change the filter to look for these extensions *at the end* of the filename and allow .patch (or possibly .diff) files to be served as plain text. ------------------------------------------------------------------------ January 30, 2005 - 12:32 : Dries The project module saves the mime-type sent by the browser when uploading the patch, and reuses that when serving the patch for download. It looks like some people upload patches with the wrong mime-type set. -- View: http://drupal.org/node/16142 Edit: http://drupal.org/project/comments/add/16142

1 0

[drupal-devel] [bug] "my account" menu item oddities
by tangent 31 Jan '05

31 Jan '05

Project: Drupal Version: 4.5.0 Component: menu system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: jasper Updated by: tangent Status: patch It seems unlikely that I am the only one to notice/realize this so I'm probably missing something but ?=/user currently redirects to the "my account" url. Therefore, shortening the link to simply "/user" would make the url generic and serve the intended purpose no? tangent Previous comments: ------------------------------------------------------------------------ November 19, 2004 - 16:03 : jasper 1. "my account" link can not be given "weight" 2. If edited, "my account" link behaves strangely: appears as separate menu in admin screen, disappears from navigation menu, etc. 3. Subitems can't be added under my account, or, strange things happen if you try to do that. ------------------------------------------------------------------------ November 25, 2004 - 06:36 : jasper Reproduce the error: 1. Assign sub-menus to "my account" menu item 2. edit "my account" menu item - capitalize "My Account" result: 1. sub-menus disappear from navigation block 2. "My Account" becomes separate Menu at the bottom of the menu administration page. I would really like to capitalize the "my account" link. Any way to do that without bugging out the menus? ------------------------------------------------------------------------ December 18, 2004 - 01:05 : jasper anybody? ------------------------------------------------------------------------ January 27, 2005 - 14:36 : JonBob Attachment: http://drupal.org/files/issues/user_13.patch (705 bytes) The "my account" menu item includes the user ID, so really cannot be customized. This is unfortunate. The attached patch corrects the mentioned bug by locking the menu item as it should be, but doesn't address the usability issue of not being able to customize the menu item. I suggest we move ahead with this patch for the 4.5 branch and, since time is short, likely 4.6. Any suggestions for how to make this item editable? I think we'd need some way to have multiple paths for a single menu item... wildcards or something. But an implementation escapes me. ------------------------------------------------------------------------ January 27, 2005 - 14:50 : Steven Perhaps we could change the path for "my account" to "user/me" or something? Link-wise it's not too hard to implement, just do "user/". ($account->uid == $user->uid ? "me" : $account->uid) or something when linking to that page. ------------------------------------------------------------------------ January 27, 2005 - 16:35 : Dries The patch did not apply cleanly against DRUPAL-4-5. I modified the patch and committed it to DRUPAL-4-5 and to HEAD. I'm leaving this issue open for a couple more days. ------------------------------------------------------------------------ January 27, 2005 - 18:21 : Boris Mann The same issue comes up with other menus that include the user ID (there are a couple, can't recall exactly which). While user/me is interesting, it would then be inconsistent with things like blog/UID and blog/UID/feed. Adding something like [uid] to menu creation might make menu code more complex, but should work. -- View: http://drupal.org/node/13184 Edit: http://drupal.org/project/comments/add/13184

2 1

[drupal-devel] [bug] Menu cache is not locale aware
by Bèr Kessels 31 Jan '05

31 Jan '05

Project: Drupal Version: 4.5.0 Component: menu system Category: bug reports Priority: normal Assigned to: chx Reported by: chx Updated by: Bèr Kessels Status: patch Correction, It was not José that submitteed the patch for locale aware menu's but Károly Négyesi, I got confused because José was the last to comment on that patch. Bèr Kessels Previous comments: ------------------------------------------------------------------------ November 26, 2004 - 08:13 : chx Attachment: http://drupal.org/files/issues/menu_locale.patch (522 bytes) If you have a multi language site, and change languages, the menu is already cached, thus it can not display the translated version. This simple patch makes the menu cache locale aware. Do I need to file the same patch against HEAD? 'Cos it applies, too. ------------------------------------------------------------------------ November 26, 2004 - 10:13 : Dries Isn't it better to flush the menu cache when someone (or the admin) switches languages? Keeps the cache size down. ------------------------------------------------------------------------ November 26, 2004 - 12:20 : chx There could be several points where the language is changed. i18n for example displays a language selection block which lets you switch anytime. Thus I think this solution is the best. ------------------------------------------------------------------------ November 26, 2004 - 13:26 : Goba See also this issue [1], which is still (again) open. The locale is also changed, when the admin changes it as the default on the admin interface, and the menu cache needs to be flushed then, and the admin needs to be redirected. I don't know how these problems are solved currently, so I would only be able to submit a placebo patch to make that issue pop up again... [1] http://drupal.org/node/11312 ------------------------------------------------------------------------ November 26, 2004 - 14:28 : Bèr Kessels I solved it by adding some logic to the swithcer function in i18n: when a language is switched the cache is flushed. The negative side of this, is that if you have hundred various roles of users reading English and one switching to German, the complete cahce is flushed. But then again, who has so many roles, that this might become an issue? ------------------------------------------------------------------------ November 27, 2004 - 10:59 : Dries I just fixed bug 11312 [2]. I don't see what's wrong with invalidating the user's menu cache when he or she switches language. Just do: <?php cache_clear_all("menu:$user->uid"); ?> [2] http://drupal.org/node/11312 ------------------------------------------------------------------------ January 30, 2005 - 20:26 : Jose A Reyero This is needed also for i18n. Invalidating cache is not a solution here, as we would need to do it also for anonymous users. So I think this is a simple solution and will work fine for both cases, user/locale and i18n. ------------------------------------------------------------------------ January 31, 2005 - 16:15 : Bèr Kessels I found out what is wrong with this: Swithcing and cache are two completely differetn things: The language is a per-session setting. The cache is a per-role setting. So any user can switch langauge on any moment, resulting in a cahce that will get flushed on nearly every page-load. And since roles and sessions are tow different thingsm we should not flush a role based cache because a variable in the session changes. The only correct option would be to have per-locale cache. (as per Jose's patch) -- View: http://drupal.org/node/13503 Edit: http://drupal.org/project/comments/add/13503

1 0

[drupal-devel] [bug] Menu cache is not locale aware
by Bèr Kessels 31 Jan '05

31 Jan '05

Project: Drupal Version: 4.5.0 Component: menu system Category: bug reports Priority: normal Assigned to: chx Reported by: chx Updated by: Bèr Kessels Status: patch I found out what is wrong with this: Swithcing and cache are two completely differetn things: The language is a per-session setting. The cache is a per-role setting. So any user can switch langauge on any moment, resulting in a cahce that will get flushed on nearly every page-load. And since roles and sessions are tow different thingsm we should not flush a role based cache because a variable in the session changes. The only correct option would be to have per-locale cache. (as per Jose's patch) Bèr Kessels Previous comments: ------------------------------------------------------------------------ November 26, 2004 - 08:13 : chx Attachment: http://drupal.org/files/issues/menu_locale.patch (522 bytes) If you have a multi language site, and change languages, the menu is already cached, thus it can not display the translated version. This simple patch makes the menu cache locale aware. Do I need to file the same patch against HEAD? 'Cos it applies, too. ------------------------------------------------------------------------ November 26, 2004 - 10:13 : Dries Isn't it better to flush the menu cache when someone (or the admin) switches languages? Keeps the cache size down. ------------------------------------------------------------------------ November 26, 2004 - 12:20 : chx There could be several points where the language is changed. i18n for example displays a language selection block which lets you switch anytime. Thus I think this solution is the best. ------------------------------------------------------------------------ November 26, 2004 - 13:26 : Goba See also this issue [1], which is still (again) open. The locale is also changed, when the admin changes it as the default on the admin interface, and the menu cache needs to be flushed then, and the admin needs to be redirected. I don't know how these problems are solved currently, so I would only be able to submit a placebo patch to make that issue pop up again... [1] http://drupal.org/node/11312 ------------------------------------------------------------------------ November 26, 2004 - 14:28 : Bèr Kessels I solved it by adding some logic to the swithcer function in i18n: when a language is switched the cache is flushed. The negative side of this, is that if you have hundred various roles of users reading English and one switching to German, the complete cahce is flushed. But then again, who has so many roles, that this might become an issue? ------------------------------------------------------------------------ November 27, 2004 - 10:59 : Dries I just fixed bug 11312 [2]. I don't see what's wrong with invalidating the user's menu cache when he or she switches language. Just do: <?php cache_clear_all("menu:$user->uid"); ?> [2] http://drupal.org/node/11312 ------------------------------------------------------------------------ January 30, 2005 - 20:26 : Jose A Reyero This is needed also for i18n. Invalidating cache is not a solution here, as we would need to do it also for anonymous users. So I think this is a simple solution and will work fine for both cases, user/locale and i18n. -- View: http://drupal.org/node/13503 Edit: http://drupal.org/project/comments/add/13503

1 0

[drupal-devel] [feature] Adjustable username crop
by tangent 31 Jan '05

31 Jan '05

Project: Drupal Version: 4.5.2 Component: node system Category: feature requests Priority: normal Assigned to: com2 Reported by: com2 Updated by: tangent Status: patch In some cases there may not be a need to truncate the output at all. We should consider using CSS to adjust the display of text rather than truncating strings to arbitrary lengths that may be less than can be viewed in a given resolution. Given the following markup <div class="container"> This is a string which may be displayed in a small area and may be truncated if the container is too small to contain it. </div> the following CSS may be used to control the overflow of the container. .container { display: block; overflow: hidden; white-space: nowrap; text-overflow: ellipsis; } Use of the proprietary (MS Internet Explorer) "text-overflow" property may be used to show ellipsis on a container with hidden content. Javascript could be used to display ellipsis on these containers for other browsers as well, though it may well not be worth doing so. Using CSS to control display in this manner gives greater control for supporting variable environments (graphical browser, wap browser, text browser) without limiting the content that I user needs to see. tangent Previous comments: ------------------------------------------------------------------------ January 27, 2005 - 11:11 : com2 Attachment: http://drupal.org/files/issues/common_0.diff (546 bytes) Some people might not like it when long usernames are cropped in the node haeder. This patch can be applied to includes/common.inc version 4.5.2 and creates a new variable 'user_name_crop' to adjust the crop length. ------------------------------------------------------------------------ January 27, 2005 - 15:32 : com2 Attachment: http://drupal.org/files/issues/common_1.diff (544 bytes) I had the numbers wrong. In the original code it was cropped at 15, not at 10 like in my diff. I also changed it a bit to avoid negative argaments. Attached the new diff from version 4.5.2. if ($object->uid && $object->name) { // Shorten the name when it is too long or it will break many tables. if (strlen($object->name) > variable_get('user_name_crop', 15)+5) { $name = truncate_utf8($object->name, variable_get('user_name_crop', 15)) .'...'; } else { $name = $object->name; ------------------------------------------------------------------------ January 27, 2005 - 16:00 : Bèr Kessels IMO this should be done in a theme, not with yet another config option. what about theme_username() ? ------------------------------------------------------------------------ January 27, 2005 - 16:07 : killes(a)www.drop.org theme_username++ format_name is one of the functions I always need to modify. theme_date would be nice, too. ------------------------------------------------------------------------ January 27, 2005 - 16:27 : Dries This should be done in the theme. -- View: http://drupal.org/node/16299 Edit: http://drupal.org/project/comments/add/16299

1 0

[drupal-devel] Bug#292647: more info
by Hilko Bengen 31 Jan '05

31 Jan '05

severity 292647 important thanks Same reason as for #292887: "drupal was not included in woody, so this upgrade issue is not RC for sarge."

2 1