- Development - lists.drupal.org

[drupal-devel] [feature] QUICKFEATURE: allow different marker types
by Goba 30 Jan '05

30 Jan '05

Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Goba Reported by: Goba Updated by: Goba Status: patch Attachment: http://drupal.org/files/issues/Drupal-allow-different-markers.patch (4.21 KB) This simple and straightforward patch adds the ability to define different types of markers (while retaining the old default behaviour of the new and required markers to look the same). Someone with enough time on his hands might be able to partition the new marker to a real new marker and a changed marker (since node_is_new() returns TRUE even if nodes changed, and not only when they are new). This is the base on which the new patch can be worked though. Goba -- View: http://drupal.org/node/16253 Edit: http://drupal.org/project/comments/add/16253

6 19

[drupal-devel] Re: [drupal:dries] /modules tracker.module
by Steven Wittens 30 Jan '05

30 Jan '05

>Modified files: > /modules tracker.module > >Log message: > - I had to use DISTINCT for the tracker.module's SQL queries. > > Doesn't the pager query need to be updated too? I think they should be SELECT COUNT(DISTINCT(n.nid)) ... - $sql = 'SELECT n.nid, n.title, n.type, n.changed, n.uid, u.name, l.last_comment_timestamp AS last_post, l.comment_count FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {users} u ON n.uid = u.uid LEFT JOIN {comments} c ON n.nid = c.nid AND (c.status = 0 OR c.status IS NULL) WHERE n.status = 1 AND (n.uid = %d OR c.uid = %d) ORDER BY last_post DESC'; + $sql = 'SELECT DISTINCT(n.nid), n.title, n.type, n.changed, n.uid, u.name, l.last_comment_timestamp AS last_post, l.comment_count FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {users} u ON n.uid = u.uid LEFT JOIN {comments} c ON n.nid = c.nid AND (c.status = 0 OR c.status IS NULL) WHERE n.status = 1 AND (n.uid = %d OR c.uid = %d) ORDER BY last_post DESC'; $sql = db_rewrite_sql($sql); $sql_count = 'SELECT COUNT(DISTINCT(n.nid)) FROM {node} n LEFT JOIN {comments} c ON n.nid = c.nid AND (c.status = 0 OR c.status IS NULL) WHERE n.status = 1 AND (n.uid = %d OR c.uid = %d)'; $sql_count = db_rewrite_sql($sql_count); $result = pager_query($sql, 25, 0, $sql_count, $uid, $uid); } else { - $sql = 'SELECT n.nid, n.title, n.type, n.changed, n.uid, u.name, l.last_comment_timestamp AS last_post, l.comment_count FROM {node} n INNER JOIN {users} u ON n.uid = u.uid INNER JOIN {node_comment_statistics} l ON n.nid = l.nid WHERE n.status = 1 ORDER BY last_post DESC'; + $sql = 'SELECT DISTINCT(n.nid), n.title, n.type, n.changed, n.uid, u.name, l.last_comment_timestamp AS last_post, l.comment_count FROM {node} n INNER JOIN {users} u ON n.uid = u.uid INNER JOIN {node_comment_statistics} l ON n.nid = l.nid WHERE n.status = 1 ORDER BY last_post DESC'; $sql = db_rewrite_sql($sql); $sql_count = 'SELECT COUNT(n.nid) FROM {node} n WHERE n.status = 1'; $sql_count = db_rewrite_sql($sql_count); Steven

1 0

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

29 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_7.patch (55.69 KB) Sorry, that was the old version, this is the right one. 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. -- 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 29 Jan '05

29 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_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. 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. -- View: http://drupal.org/node/7582 Edit: http://drupal.org/project/comments/add/7582

1 0

[drupal-devel] [bug] Article Categories display no entries for each category without "administer nodes" permission
by pyromanfo 29 Jan '05

29 Jan '05

-Project: Taxonomy Access Control +Project: Drupal -Version: 4.5.0 +Version: 4.5.0 -Component: Code +Component: taxonomy.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Zed Pobre Updated by: pyromanfo Status: patch Hey I don't know if you did this, but I just checked and it's not with my taxonomy.module patch at all, but with the core taxonomy.module. I'll move it over there. pyromanfo Previous comments: ------------------------------------------------------------------------ December 21, 2004 - 19:06 : Zed Pobre If a visiting user attempts to view more articles, all of the article categories will show 0 entries unless that user has administer nodes permission. The following error will be seen in the logs: <code> user error: query: SELECT t.tid, COUNT(DISTINCT(n.nid)) AS c FROM term_node t INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid)INNER JOIN node n ON t.nid = n.nid WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0')GROUP BY t.tid in /usr/local/share/drupal-4.5.1/includes/database.pgsql.inc on line 121. </code> This may be PostgreSQL-specific, but I have no MySQL install to test on. ------------------------------------------------------------------------ January 22, 2005 - 09:53 : Zed Pobre Attachment: http://drupal.org/files/issues/taxonomy_pgsql.patch (0 bytes) It turns out, this error is coming from taxonomy.module, despite being triggered by article.module (and nowhere else that I can see). As a first pass approximation, I think we want: <code> SELECT t.tid, COUNT(DISTINCT(n.nid)) AS c FROM term_node t INNER JOIN node n ON t.nid = n.nid INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid) WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0') GROUP BY t.tid; </code> which I have converted from the error statement I'm seeing. I've inverted the INNER JOIN order, putting node n before node_access na. This seems to generate the right results, but I'm not absolutely confident in my SQL at this hour of the morning. The attached patch to taxonomy.module implements this, fixes the problem with article.module on Postgres, and doesn't *seem* to have any negative side effects as far as I can see. Please see if it looks reasonable to you. ------------------------------------------------------------------------ January 22, 2005 - 09:54 : Zed Pobre Attachment: http://drupal.org/files/issues/taxonomy_pgsql_0.patch (0 bytes) Trying again to attach the patch. ------------------------------------------------------------------------ January 22, 2005 - 09:55 : Zed Pobre Attachment: http://drupal.org/files/issues/taxonomy_pgsql_1.patch (969 bytes) Oh, right. It's sitting on the other computer (I really wish Drupal would check for 0-byte attachments and throw a warning up somewhere to let you know that something was wrong). -- View: http://drupal.org/node/14658 Edit: http://drupal.org/project/comments/add/14658

1 0

[drupal-devel] civicspace event.module update
by Aaron Welch 29 Jan '05

29 Jan '05

There is a new version of the civicspace event module available for playing with at http://civicspace.advomatic.com. You can get additional details and give comments here: http://civicspacelabs.org/node/view/5453 This is essentially a nodeapi implementation, with the event node type removed completely. Hopefully this will allow event development to focus on good calendering functionality instead of dealing with things like dynamic fields which flexinode already handles in an excellent manner. If people like this, I'll clean the code up and hopefully have a release with drupal 4.6, since this version is built against drupal cvs. Thanks! -A

1 0

[drupal-devel] cvs log messages
by no-reply＠zind.net 29 Jan '05

29 Jan '05

File: /translations/it/system-module.po 1.6 Date: January 28, 2005 - 22:58 User: michelef Drupal 4.5.2 translation; ongoing. ---------------------------------------------------------------------- File: /modules/profile.module 1.87 /modules/user.module 1.435 Date: January 28, 2005 - 20:49 User: unconed - #16271: Clean up Admin - users - configure. ---------------------------------------------------------------------- File: /includes/pager.inc 1.41 Date: January 28, 2005 - 18:44 User: dries - Patch by Remco: <div> -> </div>. ---------------------------------------------------------------------- File: /.htaccess 1.60 Date: January 28, 2005 - 16:30 User: dries - Oops. ---------------------------------------------------------------------- File: /sandbox/junyor/import/blogger.inc 1.4 /sandbox/junyor/import/CHANGELOG 1.4 /sandbox/junyor/import/import.php 1.4 /sandbox/junyor/import/TODO 1.3 Date: January 28, 2005 - 16:25 User: junyor - Fixed bug saving users in import_create_user() (blogger.inc) - Fixed bug saving comments (import.php) - Added TODO items ---------------------------------------------------------------------- File: /sandbox/junyor/README.comment 1.2 Date: January 28, 2005 - 16:23 User: junyor Fixed typo ---------------------------------------------------------------------- File: /.htaccess 1.59 /CHANGELOG.txt 1.62 /database/database.mysql 1.168 /database/updates.inc 1.90 /modules/forum.module 1.226 /modules/taxonomy.module 1.171 Date: January 28, 2005 - 15:47 User: dries - Patch #10425 by ccourtne and walkah: added admin screens to maintain forums instead of forcing the user to add a taxonomy. ---------------------------------------------------------------------- File: /modules/block.module 1.152 Date: January 28, 2005 - 15:24 User: dries - Patch #16399 by James: fixed typo in block.module. ---------------------------------------------------------------------- File: /modules/print/INSTALL.txt 1.1.2.3 DRUPAL-4-5 Date: January 28, 2005 - 14:35 User: km km - removed hint on site logo ---------------------------------------------------------------------- File: /modules/print/po/print.pot 1.1.2.2 DRUPAL-4-5 Date: January 28, 2005 - 14:34 User: km km - changed strings back to 'printer friendly page' ---------------------------------------------------------------------- File: /modules/print/print.module 1.2.2.3 DRUPAL-4-5 Date: January 28, 2005 - 14:33 User: km km - introduced printer friendly icon ---------------------------------------------------------------------- File: /translations/it/path-module.po 1.6 Date: January 28, 2005 - 11:06 User: michelef Drupal 4.5.2 translation; help to be translated ---------------------------------------------------------------------- File: /translations/it/ping-module.po 1.6 Date: January 28, 2005 - 10:46 User: michelef Drupal 4.5.2 translation; complete. ---------------------------------------------------------------------- File: /translations/it/statistics-module.po 1.6 Date: January 28, 2005 - 10:42 User: michelef Drupal 4.5.2 translation; complete. ---------------------------------------------------------------------- File: /translations/it/general.po 1.9 Date: January 28, 2005 - 09:04 User: michelef Drupal 4.5.2 translation; complete. xtemplate_engine therms included ---------------------------------------------------------------------- File: /modules/paypal_framework/paypal_framework.mysql 1.10 Date: January 28, 2005 - 07:47 User: crackerjm applying fix http://drupal.org/node/16319 ---------------------------------------------------------------------- File: /modules/paypal_framework/paypal_framework.mysql 1.4.2.2 DRUPAL-4-5 Date: January 28, 2005 - 07:46 User: crackerjm applying fix http://drupal.org/node/16319 ---------------------------------------------------------------------- File: /modules/news_page/news_page.module 1.5 Date: January 28, 2005 - 01:04 User: MegaGrunt code clean up ---------------------------------------------------------------------- File: /modules/workspace/workspace.module 1.4.2.5 DRUPAL-4-5 Date: January 27, 2005 - 23:46 User: jvandyk sync with cvs ---------------------------------------------------------------------- File: /modules/workspace/workspace.module 1.11 Date: January 27, 2005 - 23:44 User: jvandyk use is_numeric() for validation ----------------------------------------------------------------------

1 0

[drupal-devel] [bug] files folder created with incorrect permissions
by StefanoCosta 28 Jan '05

28 Jan '05

Project: Drupal Version: cvs Component: file system Category: bug reports Priority: normal Assigned to: Morbus Iff Reported by: Anonymous Updated by: StefanoCosta Status: patch well I know this doesn't fix the problem... but i created from ftp a "/files2" directory and set it as files directory in admin > settings... it _seems_ to work. StefanoCosta Previous comments: ------------------------------------------------------------------------ September 8, 2004 - 19:04 : Anonymous Hi,- > [rreading@tribal html]$ ls -l www/files > total 12 > drwxr----- 2 nobody nobody 4096 Sep 8 12:22 pictures > drwxrwxrwx 2 rreading users 4096 Sep 8 12:33 tmp > drwxr----- 3 nobody nobody 4096 Sep 8 12:33 upload the problem is very strange, and might even be a nasty drupal bug: As of the CVS version we decided to let drupal generate the folders itself, so that Joe User needs not to use ssh or ftp to make /configure various folders. So these were generated by PHP and thus have the wrong permission. The user PHP is a differnet user in this case, and something that PHP created is inacessible to us. I am aware that this is sierverspecific, but its annoying enought for me (and probably others) not to ignore. Regards, Ber ------------------------------------------------------------------------ November 19, 2004 - 03:46 : dtan Any update on this. . ?? I recently installed the 4.5 Head and the problem still exists. Recommendations on how to actually change the permission on the folder? dtan ------------------------------------------------------------------------ November 28, 2004 - 16:53 : killes(a)www.drop.org The apache user did create this directories. So it is natural that they have the apache user "nobody" as owner. Drupal should be able to handle this just file. Can we close this report? ------------------------------------------------------------------------ November 28, 2004 - 16:58 : Bèr Kessels No, please do not close. Its a bug IMO. Because a lot of (v)hosts do not put apache in the same group as the FTP user. It will work, file uplaods etc will work for Drupal, but you, as user, cannot access the files anymore by FTP. But even worse, you cannot remove your drupal installation, without intervention of root (or the server maintainer). I had two "empty" domains with 30 MB of files taking up space for 3 months. Another domain will not let me mirror (backup) anything because of this. ------------------------------------------------------------------------ November 30, 2004 - 04:55 : svemir If PHP/apache can create these folders, it should be able to remove them as well. Perhaps an option somewhere to remove folders created by Drupal? Maybe a button next to the place in administration where the folder is specified. And also maybe the creation of the folders should be optional in the first place... ------------------------------------------------------------------------ January 20, 2005 - 02:52 : Morbus Iff This just bit me today too. Installed the banner.module, and ensured that my files/ directory was 777, with the owner as gangrene:user. Uploaded a new banner, and for various reasons, it didn't work. As I started to investigate, I found that the files/banner directory, which was "successfully" created (success determined by "I can access files uploaded in that folder via a URL) was not visible to my FTP server (at this point, proFTPd, though I've not yet tested on vsftpd). One of ProFTPds feature is to hide folders that do not have the executable bit set. The default permissions of Drupal is not to set these, per file_check_directory in includes/file.inc. So, my newly Drupal-created files/banners folder was given the permissions of (where wwwrun and nogroup are the Apache user/group). drwxrw---- 2 wwwrun nogroup 88 Jan 19 20:03 banners Since there's no execute for everyone, and since the "gangrene" user is not part of "nogroup", I was not able to see, access, or modify the newly created folder. I had to manually issue chmod('files/banners', 0761) to even look at the blasted thing. My recommendation is to change the file_check_directory chmod to 0771, which should create drwxrwx--x permissions. The extra execute permission on the "nogroup" is harmless, and allows ProFTPd to display it (in this case to "nogroup", but also perhaps to the "users" group on other systems), and the execute permission for everyone is absolutely required for me to see it in under ProFTPD. These permissions are giving any extra (or damaging) permissions to anyone, but merely following the expectations of various FTP servers concerning "what a directory is". There's a similar bug related to uploaded files, but I've not yet found the code for that one. ------------------------------------------------------------------------ January 20, 2005 - 02:55 : Morbus Iff Grrrr... "these permissions are NOT giving any extra (or damaging) permissions". ------------------------------------------------------------------------ January 20, 2005 - 03:12 : Morbus Iff To revise my previous statement, it would be best to use perms 0765, which adds "read" access to the "everyone" group. The correction is simple: if we want to /see/ the folder in FTP, then we also want to see /inside/ the folder via FTP. Again, I don't believe this change really gives extra dangerous permissions to anyone, merely grants the same permission to an authenticated FTP user (if a file upload can be seen on the web by people who know its there, then the same access should be applied to FTP users with the right knowledge [ie., un/pw]). ------------------------------------------------------------------------ January 20, 2005 - 03:29 : Morbus Iff Alrighty, hunted down the file (not directory) related problem. Unlike directories, which require us to enter a mode, file uploads are not specified, thus they take on the default umask of the web server's user (in my case, wwwgroup:nogroup, 640). This further causes problems with FTP viewing because "everyone" has no permission (in my case, since I'm a part of the "users" group, I can not view, modify, delete, mirror, etc. files that have been uploaded through Drupal). This has been tracked down to file_copy() (includes/file.inc) and the following lines, which copy the uploaded file from the temp directory ($source) to the final destination ($dest). if (!@copy($source, $dest)) { drupal_set_message(t('File copy failed.'), 'error'); return 0; } My proposal is to add an explicit chmod after the copy, granting read permission to "everyone", something to the effect of the following, which will give read and write to the webserver process, read to the webserver group, and read to everyone (ie., little old me, not of the webserver group). if (!@copy($source, $dest)) { drupal_set_message(t('File copy failed.'), 'error'); return 0; } chmod($dest, 0644). ------------------------------------------------------------------------ January 20, 2005 - 03:40 : Morbus Iff Attachment: http://drupal.org/files/issues/fileperm.patch (606 bytes) Attached is a patch for Drupal 4.5.1 that implements these changes (directory is set for 764, not 765 - the mode in my previous comment was [another] error). Last time I contributed a patch, I created a bug so please review extra carefully. ------------------------------------------------------------------------ January 20, 2005 - 17:27 : Morbus Iff In chatting about this with #drupal, I'm thinking that something more than just "sane defaults" needs to be enabled. The patch I just submitted still has one "fatal" flaw: it doesn't allow write permissions for the FTP user. This was ultimately deliberate (more chance of accepting the patch with less dangerous permission, and the assumption that a Drupal-controlled upload also offered a Drupal-controlled deletion), but still isn't ideal. Unfortunately, with SuEXEC servers and various configurations, there is no SAFE default: on one hand you're giving 666 to everyone because you have to (wwwrun:nogroup) and on the other, 666 is too much (if you're running under SuEXEC, then gangrene:users/640 is right, and 666 is wrong). I think the only way this will be truly solved is with user intervention, which means umask settings for file/directory in the "Files" section of admin/setting. This is also the same solution that Movable Type uses, which allows two special settings in its mt.cfg: # When creating files and directories, Movable Type uses umask settings to # control the permissions set on the files. The default settings for file # creation (HTMLUmask, DBUmask, and UploadUmask) are 0111; for directory # creation (DirUmask), the default is 0000. You should not change these # settings unless you are running MT under cgiwrap or suexec, or some other # scenario where the MT application runs as you; in addition, you should not # change these settings unless you know what they mean, and what they do. # # DBUmask 0022 # HTMLUmask 0022 # UploadUmask 0022 # DirUmask 0022 # # In addition to controlling permissions via umask settings, you can also # use the HTMLPerms and UploadPerms settings to control the default # permissions for files created by the system (either as output files or # uploaded files). The only real use of this is to turn on the executable bit # of files created by the system--for example, if MT is generating PHP files # that need to have the executable bit turned on, you could set HTMLPerms # to 0777. The default is 0666. You should not change these settings unless # you know what they mean, and what they do. # # HTMLPerms 0777 # UploadPerms 0777 For our needs, we'd really only need a DirectoryUmask and a FileUmask option. Is this an amicable solution to anyone? If I get you a patch, will this be implemented in Drupal 4.6? ------------------------------------------------------------------------ January 21, 2005 - 17:58 : Morbus Iff After further reflection, I'm now leaning toward: * leave the current Drupal permissions as they are (ignore my previous patch). * add GUI options to change the directory/file mode (per last comment). The GUI would be two sets of dropdown boxes, one for files and one for directories. The automatically chosen option for Files would be "read/write for Drupal only" (umask: 0640), with another option of "read/write for everybody" (umask: 0666). The directory options would be the same with the following umasks 0771 or 0777. ------------------------------------------------------------------------ January 21, 2005 - 18:26 : Morbus Iff This is a list of known possibilities that the proposed patch/UI addresses. In each case, the proposed solution above will, BY DEFAULT, create directories as 771 and files as 640. These possibilities run through a number of ownership possibilities, and any solutions in regards to an FTP user. Possibility #1 (traditional): * Drupal runs as user wwwrun, group nogroup. * FTP user IS NOT a member of group nogroup. * FTP user WILL see a Drupal-created directory. * FTP user WILL NOT be able to modify/view Drupal-created directory. * FTP user WILL NOT be able to modify/view Drupal-created files. * Solution: Toggle "read/write for everybody" for files/directories. * Pro: 777/666 is the only way this problem could be solved for FTP user. Possibility #2 (suexec/suPHP). * Drupal runs as user FTPuser, group users. * FTP user IS a member of group users. * FTP user WILL see a Drupal-created directory. * FTP user WILL be able to modify/view Drupal-created directory. * FTP user WILL be able to modify/view Drupal-created files. * Solution: None necessary. New defaults are fine. Possibility #3 (oddball configuration): * Drupal runs as user wwwrun, group users. * FTP user IS a member of group users. * FTP user WILL see a Drupal-created directory. * FTP user WILL be able to modify/view Drupal-created directories. * FTP user WILL be able to view Drupal-created files. * FTP user WILL NOT be able to modify/delete Drupal-created files. * Solution: Toggle "read/write for everybody" for files/directories. * Con: Gives "everybody" too many permissions. * Pro: This is an oddball config I've never seen. Possibility #4 (oddball configuration): * Drupal runs as user FTPuser, group nogroup. * FTP user IS NOT a member of group nogroup. * FTP user WILL see a Drupal-created directory. * FTP user WILL be able to modify/view Drupal-created directory. * FTP user WILL be able to modify/view Drupal-created files. * Solution: None necessary. New defaults are fine. * Pro: This is an oddball config I've never seen. ------------------------------------------------------------------------ January 26, 2005 - 22:12 : arnabdotorg Here's one way: Define: MIN_FILE_PERMS, MID_FILE_PERMS, MIN_DIR_PERMS, MID_DIR_PERMS (e.g. 640, 666, 711, 771) In the "admin" page (where "files" is checked for writability) 1. Create a file 2. Set it to MIN_FILE_PERMS 3. Delete it 4. If 3 fails, Set it to MID_FILE_PERMS. If does not fail, Set variable for DRUPAL_FILE_PERMS to MIN_FILE_PERMS 5. Delete it 6. if 5 fails, throw error that files are not writable/deletable. 7. If 5 does not fail, Set variable for DRUPAL_FILE_PERMS to MID_FILE_PERMS Do the same thing for directories. Also, I wont mind 3x2 checkboxes for Directories and File perms for more detailed control _in addition_ to the above algo (the above algo can be the "validate" part of the 3x2. ------------------------------------------------------------------------ January 26, 2005 - 22:19 : arnabdotorg lol, ignore this, it's useless. I'm attributing my sense of logic to it being 3:00 am here. ------------------------------------------------------------------------ January 26, 2005 - 22:29 : Morbus Iff Arnab and I talked about this on IRC, and his proposed solution sounded great ("go, go comment on the Issue and I'll make the patch. yay!"), no UI necessary, hoohah. Well, I dunno what crack I was smoking, but this doesn't actually work. If Drupal is wwwrun:nobody, then 1) will create wwwrun:nobody/640, 2) will be unnecessary, and 3) would succeed, causing MIN to be set as the DEFAULT, and morbus:users would still be out in the cold for FTP access. So, uh, ignore comment #14. It doesn't work. Back to comment #12. Thoughts? -- View: http://drupal.org/node/10658 Edit: http://drupal.org/project/comments/add/10658

1 0

[drupal-devel] Taxonomy Primer
by Ross Kendall 28 Jan '05

28 Jan '05

Since most Drupal developers are Taxonomy geeks, I thought I would share this link with you all. http://www.lexonomy.com/publications/aTaxonomyPrimer.html

1 0

[drupal-devel] [bug] Private downloads aren't working
by walkah 28 Jan '05

28 Jan '05

Project: Drupal Version: cvs Component: file system Category: bug reports Priority: critical Assigned to: Anonymous Reported by: stefan nagtegaal Updated by: walkah Status: patch Attachment: http://drupal.org/files/issues/upload-4-5.patch (1.36 KB) here's a version for 4.5 as well ... (where the SQL wasn't broken). walkah Previous comments: ------------------------------------------------------------------------ October 29, 2004 - 04:37 : stefan nagtegaal The private downloads are not working properly.. How to reproduce: Goto 'http://www.your-site.com/admin/settings' and set the 'Download method' to 'Private - files are transferred by Drupal'. Now, you make a new item (blog/story/page/poll, whatever) and upload an image. You should use this image inside your post, by doing an '' inside your post, where $path is the url which is given in the 'Attachments' form-group on the submission page. (Something like 'system/files?file=$yourfile.ext') Then click 'Preview' and do not submit the node! Now, have a look what we see.. We see that the image inside the post isn't displayed properly. The image is broken. If we ask the properties of the image it points to the right location ('system/files?file=$yourfile.ext'), but isn't displayed. If we take a look at the properties of the table under the node (attached files table with 'Attachement', and 'List') we see that the attachment is pointing to something like 'http://www.frontaal-online.com/$file-system-path/$yourfile.ext' instead of the private download url.. I'm not sure how to fix this, but I think it's a bug inside function file_create_url() which could be found inside file.inc. ------------------------------------------------------------------------ October 29, 2004 - 04:51 : stefan nagtegaal Attachment: http://drupal.org/files/issues/upload-and-body.png (19.87 KB) Screenshot after upload en filling in the body.form ------------------------------------------------------------------------ October 29, 2004 - 04:52 : stefan nagtegaal Attachment: http://drupal.org/files/issues/node-preview-and-image-properties.png (78.18 KB) Now we are previewing the node with a broken image in it.. See the properties of the image, which points to another location as the 'metallic.png' hyperlink inside the 'Attachment'-table under the node preview. ------------------------------------------------------------------------ October 29, 2004 - 04:54 : stefan nagtegaal Attachment: http://drupal.org/files/issues/properties-of-metallic-link-in-attachments-t… (53.52 KB) The properties of the 'metallic.png'-link inside the table under the node preview differs from where the source points us to.. The url inside the table works, (which is the same as in public_dowloads), but the url inside the body isn't... ------------------------------------------------------------------------ October 29, 2004 - 05:47 : stefan nagtegaal This bug is also appearing inside drupal 4.5... ------------------------------------------------------------------------ January 28, 2005 - 11:04 : walkah Attachment: http://drupal.org/files/issues/upload-private-files.patch (1.88 KB) here's a small patch to fix. basically the custom url rewriting (for when files haven't been yet saved to their final location) wasn't taking into account FILE_DOWNLOADS_PRIVATE. this fixes that (oh, and it also fixes an sql error in upload_download as well). -- View: http://drupal.org/node/12267 Edit: http://drupal.org/project/comments/add/12267

1 0