- Development - lists.drupal.org

[drupal-devel] [bug] Bad POST check in xmlrpcs.inc
by Malthus 14 Aug '05

14 Aug '05

Issue status update for http://drupal.org/node/28861 Post a follow up: http://drupal.org/project/comments/add/28861 Project: Drupal Version: cvs Component: other Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Malthus Updated by: Malthus Status: patch (code needs review) Attachment: http://drupal.org/files/issues/patch_6.txt (342 bytes) Typo which makes the check always pass. - if (!data) { + if (!$data) { Malthus

2 1

[drupal-devel] [bug] Put revisions in their own table
by killes 14 Aug '05

14 Aug '05

Issue status update for http://drupal.org/node/7582 Post a follow up: http://drupal.org/project/comments/add/7582 Project: Drupal Version: cvs Component: node system Category: bug reports Priority: normal Assigned to: killes(a)www.drop.org Reported by: killes(a)www.drop.org Updated by: killes(a)www.drop.org Status: patch (code needs review) Jose, thanks a lot for testing! "- Book outline is gone after creating a new revision" Sounds like a bug, it should not. "- I cannot delete attachments (not sure it is related to your patch or to something else)" Another bug "- Usage of the log message: Is that mensage intended for revisions or only for book outlines? Currently the field is displayed only for outlines but shown in the revision tab" Log messages are possible for nodes of type book and page as well as all nodes that are put into the book. Log messages can be added for any node type, the module author should implement those. "Btw, the db update failed for me in patch 32, so I did it manually. I didnt do a db update again after rolling back v32 and applying v33 -should I?" No, the db update should be the same. I will try to fix the bugs you found. WRT the db update I've got an announcement to make: I will not migrate old revisions. Why? 1) it is a rather complicated process that requires a lot of memory and is likely to give people on cheap hosting trouble. 2) testing it takes a lot of time. 3) I don't personally need it I will still move old revisions to an old_revisions table from where they can be converted by a later update or a custom script. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ Wed, 05 May 2004 16:25:27 +0000 : 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. ------------------------------------------------------------------------ Wed, 05 May 2004 17:06:35 +0000 : 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? ------------------------------------------------------------------------ Wed, 05 May 2004 17:57:48 +0000 : 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. ------------------------------------------------------------------------ Wed, 05 May 2004 18:37:29 +0000 : 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. ------------------------------------------------------------------------ Fri, 30 Jul 2004 19:49:33 +0000 : 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. ------------------------------------------------------------------------ Fri, 30 Jul 2004 19:54:36 +0000 : Nick Nassar Oh yeah... The patch is a diff against Drupal CVS ------------------------------------------------------------------------ Sat, 31 Jul 2004 00:00:08 +0000 : 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? ------------------------------------------------------------------------ Sat, 31 Jul 2004 01:11:59 +0000 : Anonymous Also I think that your upgrade path loses existing revisions. ------------------------------------------------------------------------ Sat, 31 Jul 2004 02:39:12 +0000 : 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. ------------------------------------------------------------------------ Sat, 31 Jul 2004 14:40:15 +0000 : 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. ------------------------------------------------------------------------ Mon, 23 Aug 2004 13:55:49 +0000 : 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. ------------------------------------------------------------------------ Mon, 23 Aug 2004 14:10:39 +0000 : 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. ------------------------------------------------------------------------ Mon, 23 Aug 2004 16:14:39 +0000 : 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. :) ------------------------------------------------------------------------ Mon, 23 Aug 2004 17:20:57 +0000 : 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 ------------------------------------------------------------------------ Mon, 23 Aug 2004 19:23:29 +0000 : 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. ------------------------------------------------------------------------ Tue, 26 Oct 2004 02:35:06 +0000 : 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. ------------------------------------------------------------------------ Wed, 27 Oct 2004 01:04:09 +0000 : 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. ------------------------------------------------------------------------ Wed, 27 Oct 2004 01:05:16 +0000 : 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. ------------------------------------------------------------------------ Wed, 27 Oct 2004 01:05:26 +0000 : 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. ------------------------------------------------------------------------ Mon, 15 Nov 2004 05:05:30 +0000 : 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! ------------------------------------------------------------------------ Wed, 24 Nov 2004 04:21:18 +0000 : 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 ------------------------------------------------------------------------ Sat, 11 Dec 2004 12:26:02 +0000 : 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. ------------------------------------------------------------------------ Mon, 13 Dec 2004 00:25:40 +0000 : 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. ------------------------------------------------------------------------ Mon, 13 Dec 2004 12:33:08 +0000 : Dries Gerhard: your patch does not apply. ------------------------------------------------------------------------ Mon, 13 Dec 2004 17:10:12 +0000 : killes(a)www.drop.org Yes, I know, that was the same version as I mailed to you earlier. ------------------------------------------------------------------------ Mon, 13 Dec 2004 21:02:06 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions.patch (52.96 KB) Ok, upüdated the patch to cvs. ------------------------------------------------------------------------ Tue, 14 Dec 2004 08:58:36 +0000 : 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. ------------------------------------------------------------------------ Tue, 14 Dec 2004 17:34:44 +0000 : Nick Nassar If you need any help getting those things fixed, just let me know. ------------------------------------------------------------------------ Tue, 14 Dec 2004 17:50:30 +0000 : 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 ------------------------------------------------------------------------ Tue, 14 Dec 2004 22:26:19 +0000 : 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. ------------------------------------------------------------------------ Wed, 15 Dec 2004 08:32:50 +0000 : Dries Attachment: http://drupal.org/files/issues/revisions-bug.png (76.06 KB) It didn't fix the aforementioned bugs. See attached screenshot. ------------------------------------------------------------------------ Thu, 06 Jan 2005 20:15:01 +0000 : 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. ------------------------------------------------------------------------ Wed, 19 Jan 2005 21:43:49 +0000 : 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. ------------------------------------------------------------------------ Wed, 19 Jan 2005 23:33:32 +0000 : 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. ------------------------------------------------------------------------ Thu, 20 Jan 2005 00:05:54 +0000 : 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. ------------------------------------------------------------------------ Thu, 20 Jan 2005 01:10:50 +0000 : 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. ------------------------------------------------------------------------ Tue, 25 Jan 2005 20:11:59 +0000 : 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. ------------------------------------------------------------------------ Sat, 29 Jan 2005 22:55:59 +0000 : 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. ------------------------------------------------------------------------ Sat, 29 Jan 2005 22:57:40 +0000 : 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. ------------------------------------------------------------------------ Mon, 31 Jan 2005 19:55:03 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_8.patch (55.71 KB) Updated once more. ------------------------------------------------------------------------ Mon, 31 Jan 2005 20:52:08 +0000 : Dries Anyone to help review/test this? ------------------------------------------------------------------------ Mon, 31 Jan 2005 21:22:36 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_9.patch (49.29 KB) Updated again, the update functions occurred twice. Thanks Bart. ------------------------------------------------------------------------ Wed, 02 Feb 2005 00:27:05 +0000 : killes(a)www.drop.org Don't know if the db I am using is corrupted or what. I still do have some didficulties. The latest patch is attached. ------------------------------------------------------------------------ Wed, 02 Feb 2005 00:27:49 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_10.patch (49.67 KB) I am probably slowly going mad ... ------------------------------------------------------------------------ Wed, 02 Feb 2005 01:54:58 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_11.patch (48.95 KB) The update issue still needs investigating. This patch is updated for cvs. ------------------------------------------------------------------------ Wed, 02 Feb 2005 20:20:51 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_12.patch (49.83 KB) Ok, here is a new version. I've solved my troubles with book.module. There are still some issues with forum module. Possibly due to inconsistent database. ------------------------------------------------------------------------ Wed, 02 Feb 2005 21:31:05 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_13.patch (49.83 KB) Turns out the drupal.org database had indeed some quirks. Please run this query in your oldest db and tell me the result: select nid,type from node where type like '%/%'; If you get a non-zero result we might need to add another security update. The patch could use still more testing, though. ------------------------------------------------------------------------ Thu, 03 Feb 2005 01:16:54 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_14.patch (49 KB) Ok, we are getting somewhere. At a first glance the update is working. There is a problem remaining: the revisions tab will be shown whether the node has revisions or not. Not sure we can/need to fix this. People with a drupal.org account can log in at http://killes.drupaldevs.org/revision/ and poke around. Your permissions will be the same as on drupal.org. Feel free to vreak everything but don't forget to file complaints here. (Note: this is only a pruned version of the drupal.org database with all project nodes and nodes with nids > 7000 dropped). ------------------------------------------------------------------------ Thu, 03 Feb 2005 04:19:14 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_15.patch (52.39 KB) There was some error in node_save and also the patches to the database.inc files got lost... ------------------------------------------------------------------------ Thu, 03 Feb 2005 07:07:27 +0000 : robertDouglass Submitting book pages doesn't work on your test site. It puts the entire content of the preview inside the body textarea. I wrote a sentence in the body and the log, and pressing preview put several lines of HTML containing both sentences in the body textarea on the preview page, plus the book page wouldn't submit. -R ------------------------------------------------------------------------ Thu, 03 Feb 2005 07:50:59 +0000 : Junyor 0 results here. I started using Drupal with version 4.4, though. ------------------------------------------------------------------------ Thu, 03 Feb 2005 23:56:18 +0000 : killes(a)www.drop.org @Junyor: Thanks, that's a good sign. Maybe somebody else has an older db to try. @robertDouglass: The first effect you describe is due to drupaldevs running on PHP 5. I am unsure why the second thing does not work. In node_save() the node object has a nid although there is none in the form. Very strange. I've enabled display of db queries on the testsite. ------------------------------------------------------------------------ Fri, 04 Feb 2005 19:17:55 +0000 : dmjossel No results here on the query: select nid,type from node where type like '%/%'; On a database that was put in place prior to Drupal 4 and is now running on 4.5.2. ------------------------------------------------------------------------ Fri, 04 Feb 2005 20:44:23 +0000 : killes(a)www.drop.org @dmjossel: thanks. @all. The strange problem I reported was apparently php 5 related. After applying Steven's php 5 patch it went away. One error is remaining: If I create a new forum topic it will be shown as part of the book on preview. Hmm, that was due to a db that got corrupted during testing so that is fixed too. Please poke around at the test site and look if you find more errors. ------------------------------------------------------------------------ Sat, 05 Feb 2005 07:16:22 +0000 : Steven By the way, I just remembered that Drupal.org has some blogs lingering on in the database even though blog.module is not enabled. Perhaps this is causing troubles? ------------------------------------------------------------------------ Sat, 05 Feb 2005 11:22:59 +0000 : Anonymous I can't see why it would. Drupal.org will need extra updates for images and project nodes because those have their own tables. GK. ------------------------------------------------------------------------ Sun, 06 Feb 2005 12:49:55 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_16.patch (52.49 KB) Updated to apply to cvs again. ------------------------------------------------------------------------ Tue, 22 Feb 2005 20:15:40 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_17.patch (49.64 KB) Updated again. All we need is a patch to upload module and an upgrade path for it. ------------------------------------------------------------------------ Fri, 04 Mar 2005 04:22:58 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_18.patch (52.31 KB) Updated once more. Moved log field from book to node_revisions table as discussed in Antwerp. upload module still missing. We need to decide under which circumstances the log field should be displayed. Should that be added to the workflow? Should it depend on the revisions setting? ------------------------------------------------------------------------ Sat, 05 Mar 2005 19:27:03 +0000 : Anonymous Attachment: http://drupal.org/files/issues/revisions_20.patch (75.52 KB) Ok, here it is: Yet another revision of this grrrrreat patch. Changes from previous versions: - supports versioning for uploaded files. A problem is that if you delete a file, it will be gone for all revisions. - the log field is now in the node_revsions table, but each module has to decide whether to show it or not. I've implemented it for the page and the book type odes. Also, the field can be edited when adding non-book nodes to the book. The log is displayed on the revisions page and if a node is moderated. - the revisions are moved to an old_revisions table to a) get the node table smaller and b) still leave the mavailable for contrib modules that want to retreive old version data. The patch has been applied to killes.drupaldevs.org/revision where it can be tested by anybody (especially people who have "site admin" rights on drupal.org) The database is from drupal.org and you should b able to log in with your pass or simply mail yourself a new one. Gerhard ------------------------------------------------------------------------ Sat, 05 Mar 2005 19:51:56 +0000 : Anonymous Attachment: http://drupal.org/files/issues/revisions_21.patch (59.42 KB) BTW, I marked this a bug because atm the revisions field can grow quite big. Neil has reported problems from some users who were not able to load some nodes due to to many large revisions. Also, som unrelated stuff crept into the patch. New version attached. ------------------------------------------------------------------------ Tue, 08 Mar 2005 05:56:01 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_22.patch (60.29 KB) Ok, I think I got it. Changes to last version: - uploads are no properly versioned. Missing are still pgsql checks and updates. ------------------------------------------------------------------------ Thu, 10 Mar 2005 16:58:41 +0000 : Anonymous Was able to get http://drupal.org/files/issues/revisions_21.patch to work with drupal-cvs.tar.gz (10 March 2005) by: - includes/database.mysql.inc: Commenting out duplicates for functions function db_begin_transaction and function db_commit_transaction - modules: node.module: Removing "'title' => $node->title," from $node_table_values variable declaration and removing "'title' => "'%s'"," from "$node_table_types" variable declaration. Happy to submit a patch if requested. I'll watch this thread. ------------------------------------------------------------------------ Fri, 11 Mar 2005 23:59:45 +0000 : killes(a)www.drop.org The duplicate function has been removed in rev 22 of this patch. Why do you think the changes in node_save are needed? Titles are saved in both tables for performance reasons. ------------------------------------------------------------------------ Sun, 13 Mar 2005 16:12:21 +0000 : jlerner Hi - I posted comment #62. The changes to node_save appear to be needed because recent patches (both 21 and 22) remove the field 'title' from table 'node'. So without the changes to node_save, node.module is broken and generates errors. Joshua ------------------------------------------------------------------------ Sun, 13 Mar 2005 16:29:42 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_23.patch (61.17 KB) Thanks, Joshua, for catching this. node:title is there to stay. ------------------------------------------------------------------------ Wed, 13 Apr 2005 16:29:23 +0000 : moshe weitzman since HEAD is open again, perhaps it is a good time to revisit this patch. once this is committed, lets address - http://drupal.org/node/11071 "node_validate does not respect group editing" ------------------------------------------------------------------------ Mon, 18 Apr 2005 15:43:42 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_24.patch (60.39 KB) Updated. ------------------------------------------------------------------------ Mon, 18 Apr 2005 16:16:32 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_24_0.patch (60.39 KB) Updated. ------------------------------------------------------------------------ Tue, 19 Apr 2005 05:19:42 +0000 : Dries I'll commit this patch later this week! If you haven't checked this patch already, I urge you to test/check it out because it will have significant impact on existing code and modules! ------------------------------------------------------------------------ Tue, 19 Apr 2005 05:21:33 +0000 : Dries Also, what do people think about the n.title being duplicated? ------------------------------------------------------------------------ Tue, 19 Apr 2005 05:26:58 +0000 : chx I won't lose any sleep because of duplicated titles... ------------------------------------------------------------------------ Tue, 19 Apr 2005 18:35:58 +0000 : killes(a)www.drop.org Let me explain why I have chosen to duplicate the title (and also the uid): If you look at all the queries in Drupal, you will find that most of them only need the title and th uid of a node. That is, by duplicating it, we save expensive joins on the node_revisions table. Due to this fact, this patch is actually a performance improvement. A note about updating contrib module: Strictly speaking they wouldn't need to be updated. They only need to if their authors decide that their info should be available for revisioning. The upgrade path for forum.module in my update.inc patch (+ the forum patch) should show you what needs to be done. I will write a note for the update page once the patch hits core. ------------------------------------------------------------------------ Sun, 24 Apr 2005 21:21:19 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_25.patch (60.38 KB) Updated to cvs. Dries: Based on some remarks in #drupal this is the last update I am going to do. Apply it or won't fix it. ------------------------------------------------------------------------ Sat, 30 Apr 2005 03:42:39 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/revisions_25.patch.patch (528 bytes) That's a big patch. I've only started looking through it. I noticed one little typo, affecting updates. A patch to your last patch is attached. I'm running with the revision patch on my dev server now happily. I like the concept. What happens if you click 'stop' on your browser in the middle of a MySQL "transaction"? I assume that kills the connection to MySQL, and the lock is freed? But this then leaves changes only partially applied? What exactly does locking/unlocking the tables buy us in MySQL? I don't see anywhere that we detect if an apply fails part way through, and thus roll back...? What am I missing? ------------------------------------------------------------------------ Sat, 30 Apr 2005 07:11:28 +0000 : Dries Jeremy: many of us are worried about the performance ramifications this patch introduces. Early experiments showed a small performance improvement (while a performance regression might be expected). More performance reports from large sites like kerneltrap.org will certainly help this patch. Mind to do a quick performance comparision and to report back with some numbers? Thanks. ------------------------------------------------------------------------ Sat, 30 Apr 2005 12:38:02 +0000 : Jeremy(a)kerneltrap.org Dries: I'm not running HEAD on kerneltrap, so this really isn't a possibility. Furthermore, until I understand why we're locking tables, I don't like it. The idea of revisions in their own tables is great. The idea of locking tables to get (without any obvious benefit) there really worries me. ------------------------------------------------------------------------ Sat, 30 Apr 2005 14:16:01 +0000 : killes(a)www.drop.org @Jeremy: Thanks for looking at the patch! Also for catching the typo. :) Did you try to upgrade your database? If yes, how did it go? One of Dries' concerns is the complexity of the upgrade. How many nodes and revisions did the db have? About database locking: This part of the patch was created by Nick and I simply continued to use it. Maybe the code should rather be: if(db_begin_transaction(array('{node}', '{node_revisions}', '{watchdog}', '{sessions}', '{files}'))) { db_query($node_query, $node_table_values); db_query($revisions_query, $revisions_table_values); db_commit_transaction(); ... } The idea is probably to avoid two updates at the same time. I don't think the locking helps if you abort the script at an inconvenient time. Rollbacks aren't implemented in all mysql versions. We could omit the db locking if deemed inappropriate. Maybe Nick can explain his ideas behind this. @Dries: I wonder who the "many of us" are. They certainly haven't spoken to me. Moshe had some reservations about the upgrade path and project module, but the time that project module abused revisions to store issue updates was long ago and his reservations were resolved. Nobody else (besides you of course and now Jeremy) has voiced reservations in a way that was audible to me. If you grep through the patch you will notice that there are only four queries which have a join on the node_revisions table. Two of them are in node_load and in the other cases the join replaced a join on the node table. The two queries in node_load are the only ones that have both a join on the node and the revisions query. Thus, loading of individual nodes might become somewhat slower. All other queries will be faster since the node table is now much smaller. Also, node loading does not have to be slower, it depends on your node table. If you had a lot of revisions and thus a large table, then the new scheme will make your queries actually faster since we do not load the revisions on each and every node load anymore. If you didn't have many revisions your node_load migth be somewhat slower. WRT to the update script Karoly pointed out that we could use multiple insert queries instead one query per revision. This would probably make the update somewhat faster. I am willing to work on this iff you declare that you will commit the patch afterwards. I'd need to know if this will work on pgsql and on all supported mysql versions before I work on it. About locking: Database locking is dog slow, at least on mysql. I was using locks in an earlier version of the upgrade script but had to remove it for (serious!) performance reasons. ------------------------------------------------------------------------ Mon, 09 May 2005 15:07:34 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_26.patch (46.45 KB) Ok, another update, cause I need it myself. I've left out the transaction stuff for now. It is in principle unrelated to this patch and should be discussed elsewhere. This also makes the patch smaller and easier to review (hint, hint). ------------------------------------------------------------------------ Mon, 09 May 2005 20:32:09 +0000 : killes(a)www.drop.org The patch contained the update functions twice. ------------------------------------------------------------------------ Mon, 09 May 2005 20:32:26 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_27.patch (39.05 KB) The patch contained the update functions twice. ------------------------------------------------------------------------ Mon, 09 May 2005 21:23:06 +0000 : Dries Got one error during the upgrade path: ALTER TABLE {book} ADD PRIMARY KEY vid (vid) FAILED ------------------------------------------------------------------------ Mon, 09 May 2005 21:26:19 +0000 : killes(a)www.drop.org This had happend to me as well, when I tested this patch. The reason is that for some reason the vid is not unique. Most likely there are some entries with vid = 0 in there. Can you check which node types those have? it always was an error in the test database. See: http://drupal.org/node/7582#comment-20678 ------------------------------------------------------------------------ Mon, 09 May 2005 21:27:06 +0000 : Dries Actually, I got 2850 errors during the upgrade. Some of these: sprintf() [function.sprintf]: Too few arguments in drupal-cvs/includes/database.inc on line 154. Some of these: Query was empty query: in drupal-cvs/includes/database.mysql.inc on line 66. And this: Unknown table 'n' in field list query: SELECT n.nid, n.vid FROM node INNER JOIN files f ON n.nid = f.nid in drupal-cvs/includes/database.mysql.inc on line 66. :-) ------------------------------------------------------------------------ Mon, 09 May 2005 21:29:19 +0000 : Dries Or this: user error: Unknown column 'log' in 'field list' query: SELECT parent, weight, log FROM book WHERE nid = 1 in drupal-cvs/includes/database.mysql.inc on line 66 ------------------------------------------------------------------------ Mon, 09 May 2005 21:52:12 +0000 : Dries The time required to generate my main page went from 902 ms (before upgrade) to 2139 ms (after upgrade). The time required to generate a forum listing (?q=forum/x) went from 1872 ms (before upgrade) to 2874 ms (after upgrade). Maybe this is because my database is not consistent as the result of the upgrade errors (yet I don't see any errors on the pages I benchmarked). ------------------------------------------------------------------------ Tue, 10 May 2005 00:24:38 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_28.patch (53.47 KB) Ok, let me get to this from the bottom to the top: - my test runs indicated a different development wrt timing. If I had gotten your results, I had stopped working on this long ago. So your results are wrong for some reason. - user error: Unknown column 'log' in 'field list' Wasn't my day, the book patch got lost. It is contained now. First -R the old patch, then apply this one. - Unknown table 'n' in field list query: Walkah found this, but I forgot to fix it. Fixed now. - I've no idea where the other queries come from. I am hoping that either your test db is borken or they are follow ups from the other ones. If you let me have your test db, I'll try some debugging. Thanks for wasting your time, too. ------------------------------------------------------------------------ Tue, 10 May 2005 05:07:31 +0000 : Dries I double-checked and the numbers don't seem to lie. I'll test some more after work on another machine to make sure it is not platform-specific. ------------------------------------------------------------------------ Wed, 11 May 2005 03:32:47 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_29.patch (54.83 KB) Ok, here I am again. What I did: 1) Ask Dries to let me have drupal.org database 2) get 400MB of SQL inserts... 3) take 23 minutes to import said data 4) Remove all image and project nodes (don't want to install their modules), 11765 nodes left 5) back up data 6) take tests on non-cached /node page (as anonymous user). ab results: -c 1 -n 25: Requests per second: 1.29 [#/sec] (mean) Connection Times (ms) min mean[+/-sd] median max Total: 663 775 179.7 689 1264 7) Do the same for the tracker page: Requests per second: 0.83 [#/sec] (mean) Total: 1182 1199 7.4 1199 1217 8) Apply my patch (rev. 28). 9) run db update and hold breath 10) update times out... 11) play back backup from 5) 12) wait 13) getting annoyed and removing cache, watchdog, and accesslog before playing in backup. 14) wait again. Understand why Dries doesn't try this patch often. Maybe a smaller DB would do for testing? 15) wait more. get really annoyed. 16) Set time limit to 18000 in update.php 17) try again 18) fails again before the second update is completed. 19) curse. 20) delete search stuff from db. Ooops, sooo much smaller... 21) import again, below 2 minutes... 22) rewrite to use extended insert. Found a bug. 23) still does not complete. Mysql logging to the rescue! 24) tid = 0? Not good. 25) Well, the update works fine till node 10834. 5595 nodes done, 6136 to go. 26) Writing shell based update script. Discovery: 24MB aren't enough. Hopefully 64 are. Nope. extended inserts for revisions are apparently not the brightest idea: Huge memory consumption. Hmm, no, all updates got through. Selecting the revisions to put them into old_revisions table screwed it. Learned about CREATE TABLE old_revisions SELECT syntax. Yay! finally. 24 MB are just not enough the update.php script seems to still break. 27) Benchmarks! /node Requests per second: 1.54 [#/sec] (mean) Connection Times (ms) min mean[+/-sd] median max Total: 632 649 40.5 636 791 /tracker Requests per second: 0.86 [#/sec] (mean) Total: 1119 1165 65.8 1160 1461 Ok: So we get an improvement for many node_loads, but none for simple selects from node. More tests can be done. 28) roll new patch Ain't Drupal fun? ------------------------------------------------------------------------ Wed, 18 May 2005 13:38:05 +0000 : Dries I did another round of tests on _another_ machine and it is not looking good: Before upgrade After upgrade ?q= (main page) 218 ms/request 340 ms/request ?q=forum (forum overview) 754 ms/request 1520 ms/request ?q=about (book page) 375 ms/request 5400 ms/request The upgrade process itself gave me a number of 'query was empty' and 'sprintf(): too few arguments' reports. Everything seems to work fine though. Looking at the ?q=about page, I see that the following query is executed twice _and_ that each time, it take more than 2 seconds to complete: SELECT n.nid, n.title, b.parent, b.weight FROM node n INNER JOIN book b ON n.vid = b.vid WHERE n.status = 1 AND n.moderate = 0 ORDER BY b.weight, n.title; --8 SHOW INDEX FROM book; +-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+ | Table | Non_unique | Key_name | Seq_in_index | Column_name | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment | +-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+ | book | 1 | book_parent | 1 | parent | A | 92 | NULL | NULL | | BTREE | | | book | 1 | nid | 1 | nid | A | 369 | NULL | NULL | | BTREE | | +-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+ 2 rows in set (0.00 sec) The book module does not appear to have a primary key? Sounds like a bad idea so I added one: mysql> ALTER TABLE book ADD PRIMARY KEY nid (nid); Query OK, 369 rows affected (0.02 sec) Records: 369 Duplicates: 0 Warnings: 0 Next, I wanted to make the vid column a unique key in all node tables: mysql> ALTER TABLE node ADD UNIQUE vid (vid); Query OK, 20392 rows affected (0.81 sec) Records: 20392 Duplicates: 0 Warnings: 0 mysql> ALTER TABLE book ADD UNIQUE vid (vid); ERROR 1062: Duplicate entry '0' for key 2 mysql> ALTER TABLE forum ADD UNIQUE vid (vid); Query OK, 10806 rows affected (0.10 sec) Records: 10806 Duplicates: 0 Warnings: 0 As you can see, it fails for the book table which makes me believe there is some inconsistent data ... I set out to fix that: mysql> SELECT nid, COUNT(nid) AS vids FROM book GROUP BY vid HAVING vids > 1; +-----+------+ | nid | vids | +-----+------+ | 871 | 2 | +-----+------+ 1 row in set (0.00 sec) mysql> SELECT title FROM node WHERE nid = 871; Empty set (0.00 sec) mysql> DELETE FROM book WHERE nid = 871; Query OK, 1 row affected (0.00 sec) mysql> ALTER TABLE book ADD UNIQUE vid (vid); Query OK, 368 rows affected (0.01 sec) Records: 368 Duplicates: 0 Warnings: 0 Looks like everything is well now. Ran some new benchmarks: Before upgrade After upgrade With indices ?q= (main page) 218 ms/request 340 ms/request 336 ms/request ?q=forum (forum overview) 754 ms/request 1520 ms/request 1531 ms/request ?q=about (book page) 375 ms/request 5400 ms/request 475 ms/request Unfortunately, we're still slower than the original code. ------------------------------------------------------------------------ Wed, 18 May 2005 21:53:31 +0000 : killes(a)www.drop.org Dries, thanks for testing it again. I do think the broken queries you observer have something to do with the bad performance after the update. Please log the queries any I will have a look at them. I've never seen any such queries. My update script also tries to create the appropriate indices, but it will of course fail if the database contains cruft. the indices for the forum are probably missing, too. I am still convinced that the patch is core worthy. ------------------------------------------------------------------------ Thu, 19 May 2005 04:36:09 +0000 : Dries It wouldn't hurt if more people would benchmark this patch. The patch's current performance worries me. Did you check your watchdog messages after upgrading the drupal.org database? Depending on your settings, errors might only be shown in the watchdog. I'll look into the remaining glitches as time permits. Thanks for your persistence in keeping this patch up-to-date. :) ------------------------------------------------------------------------ Thu, 19 May 2005 11:59:22 +0000 : killes(a)www.drop.org Dries: Can you please let me have your updated database? I want to have a look at it and try my own benchmarks with it. And yes, if I did learn something on this project is how to be persistant. ;) ------------------------------------------------------------------------ Fri, 24 Jun 2005 16:25:34 +0000 : killes(a)www.drop.org Here is an idea that occurred to me: The problem with the upgrade process is that keeping the existing revisions requires a lot of work to do. This generates a huge amount of sql queries for a large database and also requires a huge amount of memory. My suggestions is to let update.php only handle the basic upgrade, ie without old revisions. An additional module could be created that would implement a cron based approach to upgrading old revisions one node at a time. it could expose a hook to let contrib modules do their own upgrades. Dries, what do you think? (I am writing "Dries" because he seems to be the only one who is interested in getting this into core...) ------------------------------------------------------------------------ Fri, 24 Jun 2005 22:25:11 +0000 : Junyor Killes: I'm also interested in seeing this hit core. What about adding something to legacy.module to do it? ------------------------------------------------------------------------ Sun, 26 Jun 2005 21:14:54 +0000 : chx This is a sensible approach. Maybe this is the _only_ sensible approach. I have a little problem though: while the conversion is running somehow both revision handlers should be available. ------------------------------------------------------------------------ Sun, 26 Jun 2005 22:16:21 +0000 : killes(a)www.drop.org hehe, one only has to whine and bug enough and one gets some feedback. ;) @junyor: legacy.module would be a good place. my current idea is to auto-enable it in update.php and then disable it again in legacy_cron after all nodes are updated.. ;) @chx: When somebody wants to look at revisions of a node that node could be auto-updated. The only problem are contrib modules: they's need to have some hook in order to update their own data. When somebody looks at the revisions of a node than cannot be updated because the contrib module in question has no such hook, we can optionally let the user discard old revisions I guess. Dries, what do you think? ------------------------------------------------------------------------ Mon, 25 Jul 2005 16:48:51 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_30.patch (53.13 KB) Sooooo; I've updated this patch once again. Dries didn't like my idea of legacy updates so we will have an option to discard old revisions in case the update should prove difficult. Always keep a backup. ------------------------------------------------------------------------ Mon, 25 Jul 2005 17:31:12 +0000 : Bèr Kessels can we please accept and commit this patch? We can iron out any issues later. This patch is just far too big and complex to be perfect-at-once. ------------------------------------------------------------------------ Mon, 25 Jul 2005 19:22:50 +0000 : Dries @Ber: no, we can't commit this patch blindly. Data loss is a much bigger problem than a syntax error or other code glitch. We can break Drupal but we can't break their data. I've spent quite a bit of time testing the previous version of this patch and noticed significant performance degradation. Tell me, Ber, why can't you test this patch first? ------------------------------------------------------------------------ Mon, 25 Jul 2005 19:40:37 +0000 : Bèr Kessels I was not referring to not testing it. If it is just the upgrade path that proves to be cumbersome, then why can we not fix it afterwards, I.e. when everyone has had a good chance to look at it. Testing such a huge patch requires a lot of work. something no-one just does in his spare hours. We had discussions before to deal in a slightly different way with big changes; to commit them quicker and to leave the ironing out of any left overs for the community. I hooked into that discussion here. For two reasons. One is that Gerhard has spent numerous hours on maintaining this patch. The other one is that the community can be of help with ironing out issues in such a large change, much better than that Gerhard can do on his own. And yes, dataloss is very bad, but no-one should loose data, when he/she followed the instructions (backing up)... ------------------------------------------------------------------------ Mon, 25 Jul 2005 20:00:38 +0000 : Dries Ber: applying this patch takes 15 seconds. Whether I apply this patch for you, or whether you apply it yourself, it will hardly reduce the 'testing cost'. The problem is that data loss can be subtle; it might go unnoticed for a couple days. Make no mistake, I'd like to see this patch committed ASAP, but it warrants some testing. Let's test/benchmark it and commit it. ------------------------------------------------------------------------ Mon, 25 Jul 2005 20:55:03 +0000 : drumm The upgrade script already takes a long time to execute and does not provide feedback to the user about how it is doing. I plan on making the upgrade script able to spread the updates across multiple page views and give user feedback showing that progress is being made. This will hopefully make the speed of this update a moot point. I am working on this for my dayjob, CivicSpace, so it should get done "real soon now", but it should be expected to take awhile (smallish number of weeks). Please make comprimises if necessary. ------------------------------------------------------------------------ Wed, 27 Jul 2005 17:38:57 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_31.patch (48.78 KB) @drumm: the timeout isn't usually the problem. Memory consumption is. I'll look into doing the updates in chunks of 1000 nodes or so. Clousseau found a bug in the patch, updated. ------------------------------------------------------------------------ Thu, 28 Jul 2005 18:28:08 +0000 : moshe weitzman Even if the upgrade issues are resolved, we have to figure out why this patch is slowing down drupal (according to benchmarks posted here). ------------------------------------------------------------------------ Fri, 29 Jul 2005 07:51:45 +0000 : Dries We can't proceed without some additional benchmarking. I already benchmarked it on two machines (see comments above), and I'll benchmark it again after August 10. The performance impact of this patch worries me so if two other people could test and benchmark this patch extensively, that would come a long way. August 10 is close to the code freeze so some help is necessary. If Gerhard/killes reviewed/tested one of your patches, it is time to return the favor ... ------------------------------------------------------------------------ Fri, 29 Jul 2005 15:40:31 +0000 : Jose A Reyero After looking at the patch I'm not really surprised this slows down everything. I thought that the reason why we wanted revisions in their own table was in the first place to have a simpler -and faster- node table. But this patch: - Adds fields and complexity not only to the node table but also to all the main tables for data - Needs additional joins just to retrieve single nodes (which is done many times for a typical main page) - Does away with encapsulation of the revision functionality requiring other modules to handle revissions related data. So please, please, please: - Make revisions table only needed when actually using revisions - Keep that old nice thing of hiding revisions system from other modules - And what's so bad with serializing data anyway? I agree that usually there are better ways to store data. But this is one of the cases where serialization does make sense. Not the current huge serialized field, but only a new table with one serialized node per row. - In general, do it much simpler. This is way too complex and it removes more functionality than it adds. In just four words: keep it simple, please. ------------------------------------------------------------------------ Fri, 29 Jul 2005 21:25:26 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_32.patch (51.27 KB) Patch updated (v31 was buggy, and there was an update to updates.inc). I've had a discussion with Jose and he will maybe do some testing over the weekend. John is also doing some testing as we speak. ------------------------------------------------------------------------ Sat, 30 Jul 2005 16:06:59 +0000 : Nick Nassar It has been exactly one year since I first submitted this patch. You know what that means: *old patch party in IRC!* Virtual beers on me! w00t! ------------------------------------------------------------------------ Sat, 30 Jul 2005 16:08:33 +0000 : Jose A Reyero Hi again. I had a talk with killes -I have to say he enlightned me about this- and also did some testing. Sorry but I dont have a good set up to provide benchmarks. Here are my conclusions -and also some explanation for the varied results abt performance: - This is faster for simple node listings - This is slower for full node/load ....thus the performance increase/decrease for a given page should depend on the relation between the number of simple node listings (usually blocks) and the number of nodes displayed in the main page. I've also experienced some trouble with attachments -not re-created when creating a new revision- and book relations, same. I'm not sure I like this new "feature" of creating a new revision every time you rollback an old one -in Drupal 4.6 this seems not to happen. Besides these details, I think this patch is good enough -and quite a powerful approach actually- though I still feel it's bit too complex when simply dealing with nodes -without revisions-, which is what we do most of the time, and complexity will be even higher with contrib modules using still more tables, not to mention flexinode -versioning info will have to be scattered everywhere in the db. So this is not a +1 nor -1. On one side I think this is powerful. On the other side I feel this is too complex and really, it's more than I need. I would be happy with a 'revision' table having: nid, vid, data(serialized), that you can forget about when not using revisions. ------------------------------------------------------------------------ Sat, 30 Jul 2005 19:16:41 +0000 : Nick Nassar On a much more serious note that my last comment, unless I'm missing something, you've introduced a subtle bug into this patch since I left it. A and B are both updating revision 5 of a node. A calls db_next_id() and gets a revision id of 6 B calls db_next_id() and gets a revision id of 7 B updates the node's revision to 7 B adds revision 7 to the revision table A updates the node's revision to 6 A adds revision 6 to the revision table Now, you have a situation where the current revision (6) isn't actually the last revision(7). For some uses, that's not a big deal, but in the case of wikis, which is what I originally hoped to use this patch for, it creates problems. ------------------------------------------------------------------------ Mon, 01 Aug 2005 00:04:14 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_33.patch (51.27 KB) Updated the patch. updates.inc was changed and chx found a typo. @Nick: I don't think the issue is due to this patch. Drupal has currently no mechanism to not let people commit a new version of the same node, whether with or without revisions. drupal_next_id ensures that the revision IDs are unique and the later revisions will be the current one. Which revision is the current one does not depend on order of the version ID, btw. @Jose: what exactly were your problems with uploads? what do I need to do to reproduce? ------------------------------------------------------------------------ Mon, 01 Aug 2005 11:33:56 +0000 : Jose A Reyero Sorry, I dont know what I did the other day but I cannot reproduce it either :-) I applied this new patch. Problems I'm having now (Testing with "stories") - Book outline is gone after creating a new revision - I cannot delete attachments (not sure it is related to your patch or to something else) - Usage of the log message: Is that mensage intended for revisions or only for book outlines? Currently the field is displayed only for outlines but shown in the revision tab Btw, the db update failed for me in patch 32, so I did it manually. I didnt do a db update again after rolling back v32 and applying v33 -should I?

1 0

[drupal-devel] [bug] drupal base and E_NOTICE
by Thomas Ilsche 14 Aug '05

14 Aug '05

Issue status update for http://drupal.org/node/28540 Post a follow up: http://drupal.org/project/comments/add/28540 Project: Drupal Version: 4.6.0 Component: base system Category: bug reports Priority: minor Assigned to: Anonymous Reported by: hlslaughter Updated by: Thomas Ilsche Status: patch (code needs review) Attachment: http://drupal.org/files/issues/common.inc.enotice.patch.txt (668 bytes) Another patch for common inc. Thomas Ilsche Previous comments: ------------------------------------------------------------------------ Tue, 09 Aug 2005 19:19:33 +0000 : hlslaughter ok, instead of whining in the forums, i'll submit a ticket. i'm sure i'm not the first person to bring this up, and i apologize in advance if this issue is already decided/closed. but my issue is this: would it be possible to review the core modules and clean up code to the point that it would be feasible to run drupal with notices turned on (at least in a dev environment)? i think it would be pretty trivial to clean most/all of these errors from the code base. the cost of this effort would be relatively teeny, and the benefits would be great. admins would no longer panic if they happen to see these messages; with an option added to the settings screen, developers could turn on notices and would write cleaner code, which would probably result in fewer bugs in 3rd party modules; etc... you know the routine 8*) after turning on E_NOTICE in common.inc, i see the following notice messages when accessing the homepage of my site: occurences - message ----V--------------V-------- 12 notice: Undefined index: field in /var/www/html/includes/tablesort.inc on line 114. 10 notice: Undefined variable: teaser in /var/www/html/modules/opt/attachment.module on line 37. 6 notice: Undefined variable: count in /var/www/html/modules/opt/attachment.module on line 120. 6 notice: Undefined index: field in /var/www/html/includes/tablesort.inc on line 176. 4 notice: Undefined variable: rows in /var/www/html/modules/opt/attachment.module on line 352. 4 notice: Undefined variable: count in /var/www/html/modules/opt/attachment.module on line 123. 4 notice: Undefined variable: attachments in /var/www/html/modules/opt/attachment.module on line 279. 1 notice: Undefined variable: node in /var/www/html/modules/node.module on line 1950. 1 notice: Undefined offset: 0 in /var/www/html/includes/pager.inc on line 59. 1 notice: Undefined index: op in /var/www/html/modules/node.module on line 1636. 1 notice: Undefined index: node.taxonomy in /var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164. 1 notice: Undefined index: node.picture in /var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164. 1 notice: Undefined index: header.title in /var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164. 1 notice: Undefined index: header.site_slogan in /var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164. 1 notice: Undefined index: header.mission in /var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164. 1 notice: Undefined index: header.help in /var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164. 1 notice: Undefined index: from in /var/www/html/includes/pager.inc on line 53. 1 notice: Undefined index: footer.message in /var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164. 1 notice: Undefined index: footer.blocks in /var/www/html/themes/engines/xtemplate/xtemplate.inc on line 164. 1 notice: Undefined index: edit in /var/www/html/modules/node.module on line 1637. 1 notice: Undefined index: 2 in /var/www/html/modules/filter.module on line 629. 1 notice: Undefined index: in /var/www/html/includes/common.inc on line 506. 1 notice: Trying to get property of non-object in /var/www/html/modules/node.module on line 1950. again, no offense, and sorry if i'm trying to bring a dead horse back to life. ------------------------------------------------------------------------ Tue, 09 Aug 2005 19:22:44 +0000 : killes(a)www.drop.org This is being worked on for the next Drupal version. Please install yourself a test site on cvs and submit individual patches. ------------------------------------------------------------------------ Tue, 09 Aug 2005 19:32:57 +0000 : hlslaughter thanks, i'll do this. i take it patches are just posted here? do i just post a diff -N, title it patch and mark the component appropriately? ------------------------------------------------------------------------ Tue, 09 Aug 2005 19:39:08 +0000 : killes(a)www.drop.org Yes, attach here or create new issue. diff -up is what we usually use. Thanks. ------------------------------------------------------------------------ Tue, 09 Aug 2005 20:24:16 +0000 : hlslaughter Attachment: http://drupal.org/files/issues/test.patch (25.21 KB) attached is results of: harry@penny:~/cvs/third_party/drupal/drupal> diff -Naurp . /var/www/html/drupal_cvs/ > ../test.patch ok, would this type of patch be suitable? it includes fixes for all the E_NOTICE messages generated when accessing the homepage of a new, unmodified drupal installation straight from CVS (as of Aug 9 13:14) ------------------------------------------------------------------------ Tue, 09 Aug 2005 21:01:37 +0000 : killes(a)www.drop.org You are almost there. How to best prepare patches is to run Drupal directly from a cvs checkout. then you can fix the checkout and then run cvs diff -up This way the unrelated files in the patch won't be included. ------------------------------------------------------------------------ Thu, 11 Aug 2005 16:29:12 +0000 : Thomas Ilsche Attachment: http://drupal.org/files/issues/node.user.e_notice.path.txt (1.79 KB) I have a small patch for node.module and user.module that fixes 3 notices in the current HEAD. Please especially have a look at node_access_view_all_nodes, I simply do not get the current use of $node->nid (do I need some more sleep or is this really bogus?) ------------------------------------------------------------------------ Thu, 11 Aug 2005 16:36:22 +0000 : Thomas Ilsche About the newlines at eof: Sorry didn't notcied this as phped removed them manually, is there any reason for them, I got them in many drupal files? Maybe worth a fix aswell?

1 0

[drupal-devel] CivicSpace - Drupal 4.7 module porting workshop
by Kieran Lal 14 Aug '05

14 Aug '05

Hello! You have been sent a message by Kieran Lal at CivicSpace. You can view the invitation from where it originated by following this link: http://civicspacelabs.org/home/rsvp/email/a9e6b562244d1f3e22c4a92f70ff026e Here is the message: Drupal 4.7 is probably going to go into code freeze in the next several weeks. In order to facilitate an effective release cycle for contributed modules we are conducting the first of several workshops on how to port modules to Drupal 4.7. In particular, this is important for the CivicSpace community which has at least 30 contributed modules that they require to be ported to Drupal 4.7. <ul> <li>Why:At DrupalCon Portland, one of three big challenges for Drupal was to enable more developers to contribute to Drupal. This workshop is an effort to enable more developers to contribute to the Drupal platform.</li> <li>When: 11AM Pacific Coast Time, Coordinated Universal Time -7 hours; Daylight Saving Time</li> <li>Where: IRC irc.freenode.net, #drupal-porting</li> <li>Who:<ul><li> Module Maintainers</li> <li>Developers who are wiling to make patches to contributed modules</li> <li>Developers who have written custom modules</li> <li>Users who are willing to install Drupal 4.7 from CVS and test upgraded modules</li> </ul></ul> An upgrade document has been written for porting Drupal modules from 4.6 to 4.7 by Karoly Negyesi. Karoly is going to help run a workshop next Saturday at 11AM Pacific Time for two hours to facilitate developers porting their modules from 4.6 to 4.7. If you are not a module maintainer but would like to assist a maintainer you can port a module and submit a patch that has been peer reviewed to the maintainer. In most cases maintainers will be happy to recieve a working peer reviewed patch. If you are a maintainer and would like volunteers to port your modules for you please contact me Kieran at civicspacelabs dot org and I'll help to match volunteers to your modules. If you are not a programmer but are willing to install Drupal 4.7 from CVS and apply patches to modules then you can help validate that patches work. If you have a custom module that is written for previous versions of Drupal now is your chance to get live Drupal developer help to get it read to move to Drupal 4.7. Documentation is available here: <a href="http://drupal.org/node/22218">Porting modules to Drupal 4.7</a> The next porting workshop will be done at a different time to facilitate the various time zones that our developers work in. Suggestions welcome. The first workshop will be in IRC #drupal-porting on irc.freenode.net. P.S. Yes we will figure out why the RSVP module has the wrong date for this event.

1 0

[drupal-devel] CivicSpace - Event Invitation
by Kieran Lal 14 Aug '05

14 Aug '05

Hello! You have been invited to attend an event by Kieran Lal at CivicSpace. You can view the event itself by following this link: http://civicspacelabs.org/home/node/13752 You can view the full invitation by following this link: http://civicspacelabs.org/home/rsvp/email/a9e6b562244d1f3e22c4a92f70ff026e Here is the invitation message: Drupal 4.7 is probably going to go into code freeze in the next several weeks. In order to facilitate an effective release cycle for contributed modules we are conducting the first of several workshops on how to port modules to Drupal 4.7. In particular, this is important for the CivicSpace community which has at least 30 contributed modules that they require to be ported to Drupal 4.7. When: 11AM Pacific Coast Time, Coordinated Universal Time -7 hours; Daylight Saving Time Where: IRC irc.freenode.net, #drupal-porting Who: -Module Maintainers -Developers who are wiling to make patches to contributed modules -Developers who have written custom modules -Users who are willing to install Drupal 4.7 from CVS and test upgraded modules An upgrade document has been written for porting Drupal modules from 4.6 to 4.7 by Karoly Negyesi. Karoly is going to help run a workshop next Saturday at 11AM Pacific Time for two hours to facilitate developers porting their modules from 4.6 to 4.7. If you are not a module maintainer but would like to assist a maintainer you can port a module and submit a patch that has been peer reviewed to the maintainer. In most cases maintainers will be happy to recieve a working peer reviewed patch. If you are a maintainer and would like volunteers to port your modules for you please contact me Kieran at civicspacelabs dot org and I'll help to match volunteers to your modules. If you are not a programmer but are willing to install Drupal 4.7 from CVS and apply patches to modules then you can help validate that patches work. If you have a custom module that is written for previous versions of Drupal now is your chance to get live Drupal developer help to get it read to move to Drupal 4.7. Documentation is available here: <a href="http://drupal.org/node/22218">Porting modules to Drupal 4.7</a> The next porting workshop will be done at a different time to facilitate the various time zones that our developers work in. Suggestions welcome. The first workshop will be in IRC #drupal-porting on irc.freenode.net.

1 0

[drupal-devel] [bug] other valid emails fail too
by Dries 14 Aug '05

14 Aug '05

Issue status update for http://drupal.org/node/21067 Post a follow up: http://drupal.org/project/comments/add/21067 Project: Drupal Version: 4.6.0 Component: base system Category: bug reports Priority: critical Assigned to: CdnStrangequark Reported by: jwells Updated by: Dries Status: patch (code needs review) Using Drupal HEAD, the following code returns 3 x TRUE: print valid_email_address("first.last(a)somewhere.com"); print valid_email_address("first.last(a)sub.somewhere.com"); print valid_email_address("first.last(a)sub.sub.somewhere.com"); Looking at the code, valid_email_address() in HEAD is no different from the one in DRUPAL-4-6. 1.190 (dries 13-Apr-03): function valid_email_address($mail) { 1.185 (dries 28-Mar-03): $user = '[a-zA-Z0-9_\-\.\+\^!#\$%&*+\/\=\?\`\|\{\}~\']+'; 1.350 (dries 24-May-04): $domain = '(?:(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.?)+'; 1.185 (dries 28-Mar-03): $ipv4 = '[0-9]{1,3}(\.[0-9]{1,3}){3}'; 1.185 (dries 28-Mar-03): $ipv6 = '[0-9a-fA-F]{1,4}(\:[0-9a-fA-F]{1,4}){7}'; 1.185 (dries 28-Mar-03): 1.278 (dries 13-Dec-03): return preg_match("/^$user@($domain|(\[($ipv4|$ipv6)\]))$/", $mail); 1.231 (dries 16-Jul-03): } In short: I can't reproduce this problem. Dries Previous comments: ------------------------------------------------------------------------ Fri, 22 Apr 2005 05:00:42 +0000 : jwells When attemping to use a sub-domain email address for a new account, it won't pass the syntax test. We know that its really the base - but I'm sure a lot of end uers don't know. newaccount(a)research.drupal.org - this type of address will fail, though it is actually legal ------------------------------------------------------------------------ Tue, 10 May 2005 15:00:25 +0000 : CdnStrangequark Not just sub-domain emails. If you have an email address of the form: "first.last(a)somewhere.com" it will also fail even though this is perfectly valid. ------------------------------------------------------------------------ Tue, 10 May 2005 16:30:58 +0000 : CdnStrangequark After attempting to enter more emails on one of my new sites, I also discovered that the validation fails in yet more perfectly valid cases. For example: "myemail(a)somewhere.xx" where xx is the country domain code. (like .ca, or .us). Not all country codes are accepted. Here is a replacement I made for the code in common.inc: *valid_email_address($mail)* that works just great: $user = "[-a-z0-9!#$%&'*+/=?^_`{|}~]"; $domain = "([a-z]([-a-z0-9]*[a-z0-9]+)?)"; $regex = "^$user+(\.$user+)*(a)($domain{1,63}\.)+$domain{2,63}$"; //Return a 1 or 0 to mimic results of preg_match if (eregi($regex, $mail)) { return 1; } else { return 0; } The only thing this doesn't do is allow for "user@localhost" but does anyone really do that anyway? The code could be modified to do it through an alternate check on $domain though. PS: I left this post's status as active and unassigned cause I'm kinda new here and don't know the process for submitting patches and bug fixes. Hope someone can put this code in the core though cause I'm sure we're not the only ones who have run into the problem. ------------------------------------------------------------------------ Tue, 10 May 2005 21:07:29 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/valid_email.patch (1.31 KB) Our intrepid Debian developer has cooked up a patch. It is based on RFC 2822 but it needs testing. ------------------------------------------------------------------------ Tue, 10 May 2005 21:24:54 +0000 : Cvbge Please add a button 'I know my email is good, accept it!' that would be displayed when email is found invalid.

1 0

[drupal-devel] [bug] apache and drupal both compressing pages
by Dries 14 Aug '05

14 Aug '05

Issue status update for http://drupal.org/node/13224 Post a follow up: http://drupal.org/project/comments/add/13224 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: critical Assigned to: killes(a)www.drop.org Reported by: bertboerland(a)www.drop.org Updated by: Dries Status: patch (code needs review) We talked about this at OSCON and it was considered a good idea to remove the gzip compression from core. This should fix all gzip-related issues. People with big Drupal sites are likely to have access to Apache settings. For small Drupal sites, it is up to the hosting company to tune their machines. Dries Previous comments: ------------------------------------------------------------------------ Sat, 20 Nov 2004 16:22:55 +0000 : bertboerland(a)www.drop.org hi this weekend I went ahead and upgraded drupal from 4.4 to 4.5 on a machine, I dint have time/priority (in Dutch these words are almost the same :-) sooner. The core went fine and as usual lots of unpacking, configuring for all the modules I want to experiment with. After some time I tried to access the site in a -just-to-be-sure- moment from windows machine with an IE browser without being logged in. I found some strange behavior. Sometimes IE wanted to download the file, sometimes it showed the file (being a binary) and sometimes I got a valid page. I never got this from my firefox on both windows and Linux while being logged in. Later I could reproduce this on a firefox browser without being logged in. After some time / thinking I think I found out that both apache (my webserver) and drupal were compressing (gzipping) the page resulting in a browser that was able to uncompress the page once. But if it was a cached page and drupal was zipping this and Apache as well, the browser was just displaying a gzipped page. I have a rather old setup with an ancient version of apache but does this sound possible? Since I am hosting (some) other content outside the drupal dir, I would like to keep apache gzipping pages so is there an option to stop drupal from compressing pages? ------------------------------------------------------------------------ Sat, 20 Nov 2004 16:32:16 +0000 : killes(a)www.drop.org Which method use your Apache to gzip the pages? Which version is it? I find it strange that we get this report only several weeks after the release. I suspect that your particualr set up is borken. ------------------------------------------------------------------------ Sat, 20 Nov 2004 16:45:23 +0000 : bertboerland(a)www.drop.org The reason I waited so long was that I wanted others to find out this :-) Mind you, I *think* it is the fact that they are both compressing the cached pages. You might want to try it out for yourself by going to http://willy.boerland.com/myblog and in case a there is a cached page you will end up by downloading a binary that once renamed to a gz and unpacked, will give you the html file. drupal 4.5 apache: 1.3.26 (with *all* security patches) php: 4.2.3 Content-Encoding: gzip And yes, since I quess 100+ sites are already at 4.5 I think it is related to my setup. However, I understand correctly there i no way of having drupal stop sending out compressed pages? There is no configuration option? ------------------------------------------------------------------------ Sat, 20 Nov 2004 17:06:05 +0000 : killes(a)www.drop.org Yes, I get funny garbage, too. The apache version you run is the one Debian provides. It is still used very often, but still nobody but you seems to experience the problem, so I still think that somehow your setup is broken. Apache should recognize that the page is already gzipped. Do you use mod_gzip or libz? ------------------------------------------------------------------------ Sat, 20 Nov 2004 17:57:14 +0000 : bertboerland(a)www.drop.org Since this one is more or less critical for me but not for the RotW, re-setting priority to "nomal". I am using zlib btw. Did you try to save the file and unzip it? And I am correct that there is no option to disable cached paged from being zipped? ------------------------------------------------------------------------ Sat, 20 Nov 2004 18:30:23 +0000 : bertboerland(a)www.drop.org I think I "solved" this for me by changing the bootstrap.inc setting this to postponed, if no-one else experiences this problem within 3 weeks I will close this issue. ------------------------------------------------------------------------ Sat, 20 Nov 2004 18:31:19 +0000 : killes(a)www.drop.org I tried to download it now, but you must have changed something, the page renders fine. You are right, there is no option to disable gzipped cache. I'd really like to discover the reason for this problem. Normally, zlib should check for gzipped content and do not gzip it again. Drupal sends the appropriate header. ------------------------------------------------------------------------ Sat, 20 Nov 2004 18:32:25 +0000 : killes(a)www.drop.org Locking could have prevented this. ;) ------------------------------------------------------------------------ Sun, 21 Nov 2004 12:36:23 +0000 : axel(a)drupal.ru > I found some strange behavior. Sometimes IE wanted to download the file, sometimes it showed the file (being a binary) and sometimes I got a valid page. Sometimes I got same thing in Mozilla Firefox with differrent sites (not sure, but not only with Drupal) when I work trough my local proxies (I use chain of two proxies on my Debian box - one for banner deleting (privoxy) and second for offline page caching (wwwoffle)). I don't dig to details of such behaviour, but simply switch off proxies and reload browser's page help to solve problem - then I again switch on proxies and page loaded ok. May be you also access site through proxy? Then you need to check this proxy settings, I think. ------------------------------------------------------------------------ Sun, 21 Nov 2004 21:13:26 +0000 : bertboerland(a)www.drop.org Axel, no it was not related to proxys or even the browser. It was related to cached pages being zipped by drupal and sent to apache that was ziping the page as well, resulting in a zipped zipped page for the client that only once unzipped the page and hence displayed a binary ------------------------------------------------------------------------ Tue, 21 Dec 2004 17:10:12 +0000 : bertboerland(a)www.drop.org It seems like I was not the only one [1]. There is not enough information about the others setup (/please post here!/) to find a generic cause, but it is sure that some setups will cause zipped pages to be zipped. [1] http://drupal.org/node/14569 ------------------------------------------------------------------------ Thu, 03 Feb 2005 15:03:17 +0000 : leafish_dylan Has anybody found a better way to fix this problem? Disabling the storage of compressed cache pages in bootstrap.inc works, but it's ugly. Is an uncompressed cache likely to take up a lot of space in the database? If not, perhaps a global "gzip/zlib compression" option for Drupal would be simpler. ------------------------------------------------------------------------ Sat, 05 Feb 2005 14:40:16 +0000 : Anonymous As author of the gzipped cache patch I'd really appreciate if the people who experience this problem could supply detailed(!) info on their setup and the headers sent both by the browser and the server. I'd like to fix the problem (if there is one on Drupal's side) or at least document correct server settings if the problem is there. ------------------------------------------------------------------------ Sat, 05 Feb 2005 16:54:42 +0000 : bertboerland(a)www.drop.org Here some info, please ask if you need more: apache 1.3.26 mmcache (version unknown) drupal 4.5.1 Note: I didnt post (and thought abaout!) the MMcache between drupal and apache. The configuration of mmcache is extension="mmcache.so" mmcache.shm_size="16" mmcache.cache_dir="/tmp/mmcache" mmcache.enable="1" mmcache.optimizer="1" mmcache.check_mtime="1" mmcache.debug="0" mmcache.filter="" mmcache.shm_max="0" mmcache.shm_ttl="0" mmcache.shm_prune_period="0" mmcache.shm_only="0" mmcache.compress="1" headers: http://willy.boerland.com/myblog/ GET /myblog/ HTTP/1.1 Host: willy.boerland.com User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0 Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5 Accept-Language: en-us,en;q=0.5 Accept-Encoding: gzip,deflate Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7 Keep-Alive: 300 Connection: keep-alive Cookie: PHPSESSID=307f07c169e3bbcbe92d150026c97583 HTTP/1.x 200 OK Date: Sat, 05 Feb 2005 16:45:49 GMT Server: Apache-AdvancedExtranetServer/1.3.26 X-Powered-By: PHP/4.2.3 Content-Encoding: gzip Keep-Alive: timeout=15, max=100 Connection: Keep-Alive Transfer-Encoding: chunked Content-Type: text/html; charset=utf-8 ---------------------------------------------------------- ------------------------------------------------------------------------ Mon, 21 Feb 2005 10:53:02 +0000 : coma I have exactly the same problem my setup it's currently apache 2.0.52, php 4.3.10 and drupal 4.5.2. The relevant info it's: from apache: <Location /> # Insert filter SetOutputFilter DEFLATE # Netscape 4.x has some problems... BrowserMatch ^Mozilla/4 gzip-only-text/html # Netscape 4.06-4.08 have some more problems BrowserMatch ^Mozilla/4\.0[678] no-gzip # MSIE masquerades as Netscape, but it is fine BrowserMatch \bMSIE !no-gzip !gzip-only-text/html # Don't compress images SetEnvIfNoCase Request_URI \ \.(?:gif|jpe?g|png)$ no-gzip dont-vary # Make sure proxies don't deliver the wrong content Header append Vary User-Agent env=!dont-vary </Location> In php.ini: zlib.output_compression = On And I can assure you that my problem was that last element, changing zlib.output_compression to off solved the problem, and the output of all php scripts (not only drupal) it's compressed by either drupal or apache. You can test this things with wget: wget --header "Accept-Encoding: gzip,deflate" http://localhost/ The file wget drops should be a gzippped html, if you do a zcat and get a plain html it's all good, if you get another gzipped file it has been double compressed. ------------------------------------------------------------------------ Sat, 02 Apr 2005 19:38:42 +0000 : moshe weitzman i too am seeing strange behavior here. specifically, php is simply dieing after printing out the $cache->data in drupal_page_header(). truly, i don't think drupal should be storing gzipped cache, nor do i think it should perform gzip at all. these are better handled at php or apache layers. ------------------------------------------------------------------------ Sat, 23 Apr 2005 14:06:14 +0000 : mjr Apparently i am observing a new variant of that problem in one of the subsites of my 4.6 test installation: after enebaling cacheing, lynx, w3m and the gecko based browsers display garbage. lynx tells me it wants to load a file named index.html.gz. wget http://beate/drupal46/ on that site gives a readable html page. w3m -dump_head tells me: Received cookie: PHPSESSID=a6916d57d08727ab8ed945ddac4ecf2c HTTP/1.1 200 OK Date: Sat, 23 Apr 2005 13:56:19 GMT Server: Apache/2.0.53 (Debian GNU/Linux) PHP/4.3.10-10 mod_ssl/2.0.53 OpenSSL/0.9.7e mod_perl/1.999.20 Perl/v5.8.4 X-Powered-By: PHP/4.3.10-10 Set-Cookie: PHPSESSID=a6916d57d08727ab8ed945ddac4ecf2c; expires=Mon, 16 May 2005 17:29:39 GMT; path=/ Expires: Sun, 19 Nov 1978 05:00:00 GMT Last-Modified: Sat, 23 Apr 2005 13:56:19 GMT Cache-Control: no-store, no-cache, must-revalidate Cache-Control: post-check=0, pre-check=0 Pragma: no-cache Content-Encoding: gzip Vary: Accept-Encoding Connection: close Content-Type: text/html; charset=utf-8 Apparently, the site sends uncompressed pages but tells the browser it is compressed. BTW, links2 and dillo do disply the page but will not allow me to log in and disable cacheing. Any clues? Thanks Michael ------------------------------------------------------------------------ Sat, 23 Apr 2005 14:08:42 +0000 : mjr Perhaps I should mention that zlib.output_compression = Off in my php.ini ------------------------------------------------------------------------ Sat, 23 Apr 2005 18:43:30 +0000 : bertboerland(a)www.drop.org /Apparently i am observing a new variant of that problem / Please open a new bug report, since your problem is likely unrelated to this problem, open an new problem and you might want to use a link to this problem, but dont use this problem for other than descibed. ------------------------------------------------------------------------ Sat, 04 Jun 2005 23:53:22 +0000 : mjr no, it is not unrelated. After some investigation, i found out that the garbage is being stored in drupals cache. This has been reported in this thread. My present workaround is to completly disable drupals caching (and to manually delete the compressed pages from the database.) Whats the use of letting drupal compress pages and store these int its cache table? Just saving a few cycles of CPU time? On the cost of letting the web server and its client decide wether to transfer compressed or uncompressed data - they have the protocol to do that, drupal has not. Michael ------------------------------------------------------------------------ Mon, 06 Jun 2005 16:48:30 +0000 : killes(a)www.drop.org We save cpu cycles and storage space. Apache only needs to check the Encoding header which will be easy. I do not know why zlib output compression doesn't check those headers for some (or all?) people. ------------------------------------------------------------------------ Fri, 10 Jun 2005 23:32:40 +0000 : bluec I have the same problem with Drupal 4.6.1: If the site delivers a cached page it is gziped twice and the result is garbage in my browser window. I tried two differnet browsers: Firefox and Opera and it's the same problem with both of them. I recorded the HTTP headers: - Cache turned on - zlib-compression turned on - Result: Garbage http://kim.sshtun.v63.org:22003/index.html GET /index.html HTTP/1.1 Host: kim.sshtun.v63.org:22003 User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4 Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5 Accept-Language: de-de,en;q=0.5 Accept-Encoding: gzip,deflate Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7 Keep-Alive: 300 Connection: keep-alive Referer: http://kim.sshtun.v63.org:22003/fachschaft/ Cookie: PHPSESSID=6ddbf3d5a32d-CHANGED-BY-CHRIS HTTP/1.x 200 OK Date: Fri, 10 Jun 2005 23:16:08 GMT Server: Apache/2.0.54 (Gentoo/Linux) mod_ssl/2.0.54 OpenSSL/0.9.7e PHP/5.0.4 X-Powered-By: PHP/5.0.4 Expires: Mon, 26 Jul 1997 05:00:00 GMT Last-Modified: Fri, 10 Jun 2005 23:11:59 GMT Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0 Pragma: no-cache Set-Cookie: PHPSESSID=6ddbf3d5a32d20-CHANGED-BY-CHRIS; expires=Mon, 04 Jul 2005 02:49:28 GMT; path=/ Etag: "03cff5fcc105e72dfc2671d3ffdb285c" Content-Encoding: gzip Content-Length: 5024 Keep-Alive: timeout=15, max=95 Connection: Keep-Alive Content-Type: text/html; charset=utf-8 Now I turned off compression by setting in my .htaccess php_flag zlib.output_compression On and the result is OK: http://kim.sshtun.v63.org:22003/index.html GET /index.html HTTP/1.1 Host: kim.sshtun.v63.org:22003 User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4 Accept: text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5 Accept-Language: de-de,en;q=0.5 Accept-Encoding: gzip,deflate Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7 Keep-Alive: 300 Connection: keep-alive Referer: http://kim.sshtun.v63.org:22003/index.html Cookie: PHPSESSID=6ddbf3d5a32d20-CHANGED-BY-CHRIS HTTP/1.x 200 OK Date: Fri, 10 Jun 2005 23:17:08 GMT Server: Apache/2.0.54 (Gentoo/Linux) mod_ssl/2.0.54 OpenSSL/0.9.7e PHP/5.0.4 X-Powered-By: PHP/5.0.4 Expires: Mon, 26 Jul 1997 05:00:00 GMT Last-Modified: Fri, 10 Jun 2005 23:11:59 GMT Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0 Pragma: no-cache Set-Cookie: PHPSESSID=6ddbf3d5a32d208-CHANGED-BY-CHRIS; expires=Mon, 04 Jul 2005 02:50:28 GMT; path=/ Etag: "03cff5fcc105e72dfc2671d3ffdb285c" Content-Encoding: gzip Vary: Accept-Encoding Content-Length: 5054 Keep-Alive: timeout=15, max=100 Connection: Keep-Alive Content-Type: text/html; charset=utf-8 Turning off the cache is a solution as well. As soon as cache and zlib are operating the result is garbage. If you need any other information just let me know. I can turn on compression at any time and test other solutions. Chris ------------------------------------------------------------------------ Mon, 27 Jun 2005 18:10:12 +0000 : aries The same. Interesting, only one page doing this, all others are not. -- Aries http://aries.mindworks.hu ------------------------------------------------------------------------ Thu, 07 Jul 2005 14:46:34 +0000 : fago quite strange. i never had problems with this, then i upgraded from 4.6.1 to drupal 4.6.2... now i must turn off the cache, or i've the troubles mentioned here... (i haven't touched the apache or php config in the meanwhile) ------------------------------------------------------------------------ Thu, 14 Jul 2005 09:47:32 +0000 : bertboerland(a)www.drop.org yet another one [2], closed than one, raised importance here. [2] http://drupal.org/node/25326 ------------------------------------------------------------------------ Sat, 16 Jul 2005 19:52:26 +0000 : bertboerland(a)www.drop.org killes sigested that is want apache that compressed the page but drupal and php was the first one. after some testing, i found out he was correct! to solve this 1) either comment out the gzip lines in bootstrap.inc 2) put /etc/php.ini "zlib.output_compression = On" to OFF (real) Fix needed. Drupal should look if the page was zipped in the first place.... ------------------------------------------------------------------------ Fri, 29 Jul 2005 12:17:46 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/gzip.patch (2.45 KB) Here's a patch that needs some testing. ------------------------------------------------------------------------ Fri, 29 Jul 2005 13:18:51 +0000 : Dries Why do we set $do_cache to TRUE in the nested if's? $do_cache is already set to TRUE higher up so we're just overwriting TRUE with TRUE? Also, can we rename $do_cache to $cache? That is more Drupal-ish. Does that solve all apache-Drupal-caching problems or only some? I vaguely remember we agreed to add a setting, because that was perceived the only True Solution. ------------------------------------------------------------------------ Fri, 29 Jul 2005 18:22:04 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/gzip_0.patch (2.46 KB) I've updated the patch. The patch is intended to cure all the problems we had. It needs testing, though. I don't recall to agreeing that a setting would be needed. Just for the record: Steven measured the ratio of serving gzipped cached pages to re-unzipped pages on drupal.org to be about 5.1:1. That is from 6 page requests which we serve from the cache only one needs the cache to be unzipped. This is probably due to crawlers who only speak http 1.0. ------------------------------------------------------------------------ Sat, 30 Jul 2005 14:33:42 +0000 : bertboerland(a)www.drop.org people who expierenced this problem, please try the patch of killes and report here if successful or not. i will patch as well ------------------------------------------------------------------------ Mon, 01 Aug 2005 14:55:47 +0000 : moshe weitzman In my opinion, this is a case of misplaced optimization. Very little CPU is required to gzip a document. Why else would Apache and PHP provide an option to do this *for every page*. PHP knows that all its pages will be dynamic yet it still offers this option. My recommendation is this feature entirely, and take the win in simplicity/code reduction. Furthermore, we won't have any more issues like this one, which has lingered unfixed for 9 months. Admins who want gzip will elect to do so in their php.ini or .htaccess. ------------------------------------------------------------------------ Fri, 05 Aug 2005 13:46:03 +0000 : Arto If the caching mechanism is to be patched, that'd be a good time to also tackle the problem that it's currently unusable on PostgreSQL: http://drupal.org/node/26369 ------------------------------------------------------------------------ Fri, 05 Aug 2005 14:56:47 +0000 : fago i don't have this issue any more. since i'm using a new server running apache2 with deflate instead of apache1 with mod-gzip everything is fine. unfortunately, i've no access to the old server (for testing) any more.

1 0

[drupal-devel] [bug] Category selection does not work with Movable Type blogapi
by Dries 14 Aug '05

14 Aug '05

Issue status update for http://drupal.org/node/24030 Post a follow up: http://drupal.org/project/comments/add/24030 Project: Drupal Version: 4.6.2 Component: blogapi.module Category: bug reports Priority: normal Assigned to: walkah Reported by: mrmachine Updated by: Dries Status: patch (code needs review) I committed this patch to DRUPAL-4-6. I won't commit it to HEAD. We need a better fix for this. Dries Previous comments: ------------------------------------------------------------------------ Tue, 31 May 2005 22:38:14 +0000 : mrmachine Using a blog client (linux - either Drivel or BlogTK) and the Moveable Type api, you can retrieve categories and select one for your post, but once the post is submitted, it fails to allocate the category - making the use of a blog tool pretty useless. ------------------------------------------------------------------------ Tue, 31 May 2005 22:45:51 +0000 : laura s I use ecto, set to MoveableType API,but so far I'm not able to repeat this problem. Could it be the client? I was also able to set Drupal's BlogAPI to the MetaWeblog setting and it seemed to work fine -- even with ecto set to MoveableType API. You might be able to try that as a workaround. ------------------------------------------------------------------------ Thu, 02 Jun 2005 12:04:12 +0000 : mrmachine I tried ecto on my son's Windows box, and yes - it does set categories correctly ... too bad there's no linux version but anyway, i tried changing Drupal's blogapi to Metaweblog, and kept Drivel and BlogTK set to Moveable Type API. It didn't work for Drivel, but it did for BlogTK ... thanx for the hint! ------------------------------------------------------------------------ Sun, 05 Jun 2005 14:36:44 +0000 : fflewddur I've been trouble-shooting this issue using w.bloggar on Windows and Drivel on Linux. w.bloggar is able to post using the Movable Type API, but the category selection only works if you click "Post", not "Post and Publish". Drivel, by default, publishes all posts, which is why the category never shows up when using Drivel with Drupal. To post entries with categories to an official Movable Type implementation, the client must first post the entry, then call mt.setPostCategories, and then call mt.publishPost. The mt.publishPost call is what seems to be confusing Drupal, as it doesn't appear to differentiate between a post that has been published with one which has. Movable Type does not publicly display posts which have not been published; it considers them as drafts which the author has not yet finished. Thus, Movable Type servers require the mt.publishPost call. I have Ethereal traces of several example posts to Drupal using both w.bloggar in "post" and "post and publish" mode, as well as Drivel. If you think they'd be useful in debugging, let me know and I'll forward them on to you. Cheers, Todd ------------------------------------------------------------------------ Thu, 09 Jun 2005 03:57:11 +0000 : mrmachine has anyone had a chance to look at this since Todd debugged it? ------------------------------------------------------------------------ Mon, 13 Jun 2005 20:56:58 +0000 : mrmachine can someone have a look at this? ------------------------------------------------------------------------ Tue, 14 Jun 2005 00:05:25 +0000 : walkah Attachment: http://drupal.org/files/issues/blogapi-publish-post.patch (634 bytes) having a look - it seems that mt.publishPost may well have been wiping the term info set by setCategory. As I only have ecto available to me currently, I can't test this patch... but if I understand the debugging work correctly - this should fix it. (the difference being ecto and MarsEdit - the two that I have access to currently - don't call publishPost). please confirm this fixes things... thanks. (I'll test more when I'm home again at the end of the week). ------------------------------------------------------------------------ Tue, 14 Jun 2005 23:06:29 +0000 : mrmachine beauty! this has fixed it for me! thanx shayne ------------------------------------------------------------------------ Wed, 15 Jun 2005 04:29:48 +0000 : baudolino This is not fixed until a decision about whether to commit this patch is made. Reverting this to patch status. ------------------------------------------------------------------------ Sun, 19 Jun 2005 20:25:55 +0000 : Dries Actually, this is an annoying property of node_save() ... I guess you can't use node_invoke_nodeapi($node, 'insert') or node_invoke_nodeapi($node, 'update')? ------------------------------------------------------------------------ Thu, 23 Jun 2005 17:53:32 +0000 : walkah i'm not sure that'd make a difference, would it? isn't the issue that $node->taxonomy is unset? ------------------------------------------------------------------------ Wed, 20 Jul 2005 11:56:50 +0000 : mrmachine i upgraded to 4.6.2 (a tad late though - i'd been hacked), and am now back to the original problem (i.e. - using Drivel or BloGTK, i am unable to set the category - i can choose one, but it doesn't stick) ... i'm guessing it's because this is "not fixed" and so wasn't committed with the 4.6.2 release. am i able to use the same patch to "not fix" 4.6.2? in doing so, i won't mess up any 4.6.2 security patches, will i? shayne ------------------------------------------------------------------------ Sat, 23 Jul 2005 21:26:06 +0000 : mrmachine "am i able to use the same patch to "not fix" 4.6.2? " no. no i can't. i'd really like to test any possible fixes. are there any? ------------------------------------------------------------------------ Mon, 25 Jul 2005 00:18:51 +0000 : mrmachine has anyone managed to test this patch on 4.6.2? i haven't been able to get it working, so i'm back to a patched 4.6.1 blogapi.module ... i just hope this doesn't mean i'm still open to security vulnerabilities of 4.6.1 as my site was defaced only last week ... though i'm sure the vulnerability was only in the xmlrpc.php file.

1 0

[drupal-devel] [feature] introduce a "cron" user to fix various cron related issues
by robertDouglass 14 Aug '05

14 Aug '05

Issue status update for http://drupal.org/node/5380 Post a follow up: http://drupal.org/project/comments/add/5380 Project: Drupal Version: cvs Component: user system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: ax Updated by: robertDouglass Status: patch (code needs review) Eric, that makes sense. Thanks for the explanation. robertDouglass Previous comments: ------------------------------------------------------------------------ Fri, 23 Jan 2004 23:33:00 +0000 : ax we currently have some problems caused by the undefined state of $user in the cause of cron runs. sometimes it's "Anonymous", sometimes it picks up the currently logged in user (i think). I stumbled upon this when reading issues closed by "Anonymous" caused by the automatic cron closure when issues are fixed. i always think "isn't it strange that "Anonymous" can close issues just as she feels like?" there are quite some related issues - search this site for "cron user". couldn't a cron user solve these problems? a la if (REQUEST_URI) == cron.php then $user = <cron_user>, with <cron_user> having some special properties such as showing "Automatically" for closed issues, being possible to excluded in statistics, putting the rss author from imported rss nodes into nodes author field, having special access checks etc. i didn't think about this much, but thought it is worth filing here. please discuss, elaborate - and don't flame me if i'm too off ;) ------------------------------------------------------------------------ Wed, 11 Feb 2004 05:04:29 +0000 : Chris Johnson Sounds like a good idea to me. It's one of many things I'd like to change if I get time to work on it. ------------------------------------------------------------------------ Mon, 27 Sep 2004 19:25:42 +0000 : Philippe Couln't we just write this in cron.php: <?php $cronuser = variable_get('cron_cronuser','nobody'); if ($cronuser = 'nobody'){ $user = user_load(array()); //I believe this results in Anonymous?? }else{ $user = user_load(array('name' => $cronuser)); } ?> We can set the variable cron_cronuser in a page like admin/cron. If the site administrator doesn't set a value, cron uses Anonymous for everything (not some user that happens to be logged on). If a valid user name is set, the site admin can give the cron user special privileges. ------------------------------------------------------------------------ Fri, 01 Oct 2004 18:41:16 +0000 : Philippe Setting the user like this in cron.php sends a cookie. (How? Where? Can we prevent this?) So everybody can get "cron" privileges by loading cron.php once. We could fix this by making cron.php accessible only from localhost (in .htaccess). Any ideas? ------------------------------------------------------------------------ Fri, 01 Oct 2004 21:11:37 +0000 : Philippe Attachment: http://drupal.org/files/issues/cron_uid.patch (3.62 KB) This patch lets the admin select a UID for cron in the admin/settings page. The variable name in which this is stored is 'cron_uid'. When cron.php is loaded, it sets a variable $cron_is_running. If this variable is set, no session will be opened (and no cookies will be sent). The cron page will use the Anonymous user (uid==0) if cron_uid is not set. Note that the site admin should create a "cron" user before setting the UID. ------------------------------------------------------------------------ Sun, 13 Mar 2005 19:18:05 +0000 : Chris Johnson Attachment: http://drupal.org/files/issues/cron_uid2.patch (3.19 KB) Previous patch still applies cleanly to HEAD and code algorith looks good on inspection. However, there are some style problems and one ugly bit using StdClass(), so I have re-rolled the patch. ------------------------------------------------------------------------ Sun, 13 Mar 2005 19:37:14 +0000 : killes(a)www.drop.org I like the idea, some comments, though: +if (!$cron_is_running) { + session_start(); +} Is this neccessary? 'User ID '.$cron_uid.' does not exist' ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:11:09 +0000 : killes(a)www.drop.org 'User ID '.$cron_uid.' does not exist' < == needs to be made translatable. Setting to active. ------------------------------------------------------------------------ Mon, 14 Mar 2005 05:56:17 +0000 : Chris Johnson Attachment: http://drupal.org/files/issues/cron_uid3.patch (3.29 KB) Ooops. Sorry I missed that text that needed translating. New patch attached with that bit corrected. Yes, I think the if (!$cron_is_running) code is needed to keep the cron user from creating many unneeded sessions. ------------------------------------------------------------------------ Mon, 14 Mar 2005 09:48:51 +0000 : stefan nagtegaal I am all for the idea of introducing a special id for running the cronjobs, but the approach you took here is far from perfect imo.. After looking at your patch, there are several questions and remarks that came up to my mind: I agree with Gerhard that the $cron_is_running is probably unneeded, I think it's better todo: <?php if (!variable_get('cron_busy', true)) { session_start(); } ?> I think that this is much to technical. Severa newbies will probable ask what a user id is, or what cron jobs are.. From both usability and userfriendlyness this patch is a big -1 from me.. I would suggest todo the following: - Make a new user in the db called 'Cron' with a user id -1, or something like that.. That way you don't have to add extra options to the administration, and the way of implementing should be much cleaner. Also does it improves usability... What do you think? ------------------------------------------------------------------------ Mon, 14 Mar 2005 13:11:44 +0000 : Anonymous Hi, The functionality is great, but your interface is not. IMO you should just choose good defaults and leave it with that. Why would users want to choose "UIDs" ofr "cron Jobs". Joe user should not have to think about UIDs and Crons. IT should Just Work[tm]. I know there are lots of places in Drupal where this is not the case either, but we are working on that. So when we introduce new functionality we should do it good at once. Why not go for Stefans suggestion of choosing a cron UID and jsut leave it like that. UIDs are not really used anyway, so you can choose just any UID (just the next one available, most often will be '2'). Bèr ------------------------------------------------------------------------ Mon, 14 Mar 2005 13:45:54 +0000 : javanaut "Make a new user in the db called 'Cron' with a user id -1" Just on a technical note, the database type for uids is "unsigned" and -1 won't work. ------------------------------------------------------------------------ Mon, 14 Mar 2005 19:08:48 +0000 : killes(a)www.drop.org @Chris: I am pretty sue that this patch will not be usefull if the cron user does not get a session. Drupal relies on the session to authenticate the user. If you are worried about too many sessions, you should make sure the session is clossed at the end of the run. his could be done in cron.php. @Stefan, Ber: I think we should introduce just a standard user and assign the "authneticated user" role to him. The set the variable to that uid so we can check which user it is. The problem is that we cannot create the user from database.mysql beacuse the next uid would be No. 1 which is reserved. So we would need to create the user on the first visit to the admin/settings page. We could also use drupal_set_message to inform the admin of this. We should also extent the relevant help text. While we are at it we could make the anon user use the db tabe for its name too and not a variable. ------------------------------------------------------------------------ Mon, 14 Mar 2005 20:33:00 +0000 : Chris Johnson Looks like we are sort of in a corner. Really the cron user should be in the database at install time, but we have to reserve uid 1 for the first user. Maybe there is another way to do that without having the first entry to admin create the cron user? That seems hackish to me. I agree that the anonymous user should also be completely coded into the database, and not a variable. The current methods causes anomolous behaviors. I, in fact, wrote several patches to do that a long time ago (http://drupal.org/node/5639) I looked at updating that patch a couple of months ago, but the new unregistered user comment capability where the user can enter his name made the code a lot more complicated. Maybe I will get motivated to try it again. If only my patch had been applied the first time! :-) ------------------------------------------------------------------------ Mon, 14 Mar 2005 21:21:12 +0000 : Olen How about creating a bunch of "reserved" users (like most linux distributions do)? uid 1 = root uid 2 = cron uid 3 - 8 = reserved for later use uid 9 = anonymous And then let the admin log in as root the first time. We still let him/her change the username of any of these users, so it should not cause to many problems for new installations. Older ones would either just have to live with it, or do a lot of 'UPDATE module_table SET uid=X WHERE uid=2'-queries. ------------------------------------------------------------------------ Mon, 14 Mar 2005 22:33:32 +0000 : Bèr Kessels What about following the linux thing, and start counting at 500 for normal users? That way people will recognise it, and numbers do not matter anyway. I mean, there is really no difference between 4 and 500, other than thats its a different number. 500 makes just as much sense as 10, or 2 or 4. Bèr ------------------------------------------------------------------------ Tue, 15 Mar 2005 12:59:18 +0000 : Philippe The reason why I created the "ugly" user interface, and let the site admin choose a cron-user ID in the settings page is because of backward compatibility with existing installations. (guess what: my site IS an existing installation) Perhaps another option is to give special users a user ID close to 2147483647 (MAXINT). That way it won't interfere with existing users. ------------------------------------------------------------------------ Thu, 16 Jun 2005 23:12:05 +0000 : degerrit At the risk of being publicly humiliated, and in the spirit of "there's no such thing as a stupid question" ... what exactly is the (security?) issue with adding: $user = user_load(array('uid' => 1)); in cron.php? I have two sites with node_privacy_by_role (NPBR) enabled, and cron does not index all the pages as a consequence (except when it is given a user id with full access like above). It took me some puzzling to figure out why the sites seemed stuck at "11% of the site has been indexed" or something similar. Of course, now a non-authenticated user can see excerpts of "private" pages through the search, which could be a big deal. Anyway, being able to choose a search user would solve the problem with NPBR as well it seems. ------------------------------------------------------------------------ Fri, 12 Aug 2005 15:59:10 +0000 : robertDouglass Why do we need a user besides uid=0 for cron? Just because the project module writes anonymous on certain closed issues? If cron runs with uid=0 most of the time anyway, why don't we just hardcode that in instead of creating a new type of user? ------------------------------------------------------------------------ Sun, 14 Aug 2005 07:57:09 +0000 : ejort @robertDouglass: Why do we need a user besides uid=0 for cron? There are genuine use cases for cron running with greater permissions than those provided by uid=0, and having a specific cron user allows the administrator to determine exactly which permissions the cron user gets. For instance I just spent half a day figuring out why my simplenews installation was mailing multiple empty messages to subscribers plus 1 working message. My simplenews mail messages are only accessible by users with the 'authenticated user' role via taxonomy_access, my simplenews install sends for 20 seconds on message submit and then continues sending later via cron. When sending via cron the message body couldn't be accessed by the anonymous user and so an empty message got sent. It was only when a logged in user initiated a send run that a proper message was sent. Without the ability to assign permissions to a cron user, I have to allow access to my newsletters to anonymous users. So, that's my +1 for this functionality. @killes: . So we would need to create the user on the first visit to the admin/settings page. I agree that this variable ('cron_uid') should be set automatically without requiring any admin intervention. Can't we create this user in update.php? The variable defaults to 0 which is the current behaviour in any case. Finally, I think that the logic to not create a session for the cron user should be removed from this patch. Instead we should destroy the session at the end of the cron run. Cheers, Eric

1 0

[drupal-devel] [feature] introduce a "cron" user to fix various cron related issues
by ejort 14 Aug '05

14 Aug '05

Issue status update for http://drupal.org/node/5380 Post a follow up: http://drupal.org/project/comments/add/5380 Project: Drupal Version: cvs Component: user system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: ax Updated by: ejort Status: patch (code needs review) @robertDouglass: Why do we need a user besides uid=0 for cron? There are genuine use cases for cron running with greater permissions than those provided by uid=0, and having a specific cron user allows the administrator to determine exactly which permissions the cron user gets. For instance I just spent half a day figuring out why my simplenews installation was mailing multiple empty messages to subscribers plus 1 working message. My simplenews mail messages are only accessible by users with the 'authenticated user' role via taxonomy_access, my simplenews install sends for 20 seconds on message submit and then continues sending later via cron. When sending via cron the message body couldn't be accessed by the anonymous user and so an empty message got sent. It was only when a logged in user initiated a send run that a proper message was sent. Without the ability to assign permissions to a cron user, I have to allow access to my newsletters to anonymous users. So, that's my +1 for this functionality. @killes: . So we would need to create the user on the first visit to the admin/settings page. I agree that this variable ('cron_uid') should be set automatically without requiring any admin intervention. Can't we create this user in update.php? The variable defaults to 0 which is the current behaviour in any case. Finally, I think that the logic to not create a session for the cron user should be removed from this patch. Instead we should destroy the session at the end of the cron run. Cheers, Eric ejort Previous comments: ------------------------------------------------------------------------ Fri, 23 Jan 2004 23:33:00 +0000 : ax we currently have some problems caused by the undefined state of $user in the cause of cron runs. sometimes it's "Anonymous", sometimes it picks up the currently logged in user (i think). I stumbled upon this when reading issues closed by "Anonymous" caused by the automatic cron closure when issues are fixed. i always think "isn't it strange that "Anonymous" can close issues just as she feels like?" there are quite some related issues - search this site for "cron user". couldn't a cron user solve these problems? a la if (REQUEST_URI) == cron.php then $user = <cron_user>, with <cron_user> having some special properties such as showing "Automatically" for closed issues, being possible to excluded in statistics, putting the rss author from imported rss nodes into nodes author field, having special access checks etc. i didn't think about this much, but thought it is worth filing here. please discuss, elaborate - and don't flame me if i'm too off ;) ------------------------------------------------------------------------ Wed, 11 Feb 2004 05:04:29 +0000 : Chris Johnson Sounds like a good idea to me. It's one of many things I'd like to change if I get time to work on it. ------------------------------------------------------------------------ Mon, 27 Sep 2004 19:25:42 +0000 : Philippe Couln't we just write this in cron.php: <?php $cronuser = variable_get('cron_cronuser','nobody'); if ($cronuser = 'nobody'){ $user = user_load(array()); //I believe this results in Anonymous?? }else{ $user = user_load(array('name' => $cronuser)); } ?> We can set the variable cron_cronuser in a page like admin/cron. If the site administrator doesn't set a value, cron uses Anonymous for everything (not some user that happens to be logged on). If a valid user name is set, the site admin can give the cron user special privileges. ------------------------------------------------------------------------ Fri, 01 Oct 2004 18:41:16 +0000 : Philippe Setting the user like this in cron.php sends a cookie. (How? Where? Can we prevent this?) So everybody can get "cron" privileges by loading cron.php once. We could fix this by making cron.php accessible only from localhost (in .htaccess). Any ideas? ------------------------------------------------------------------------ Fri, 01 Oct 2004 21:11:37 +0000 : Philippe Attachment: http://drupal.org/files/issues/cron_uid.patch (3.62 KB) This patch lets the admin select a UID for cron in the admin/settings page. The variable name in which this is stored is 'cron_uid'. When cron.php is loaded, it sets a variable $cron_is_running. If this variable is set, no session will be opened (and no cookies will be sent). The cron page will use the Anonymous user (uid==0) if cron_uid is not set. Note that the site admin should create a "cron" user before setting the UID. ------------------------------------------------------------------------ Sun, 13 Mar 2005 19:18:05 +0000 : Chris Johnson Attachment: http://drupal.org/files/issues/cron_uid2.patch (3.19 KB) Previous patch still applies cleanly to HEAD and code algorith looks good on inspection. However, there are some style problems and one ugly bit using StdClass(), so I have re-rolled the patch. ------------------------------------------------------------------------ Sun, 13 Mar 2005 19:37:14 +0000 : killes(a)www.drop.org I like the idea, some comments, though: +if (!$cron_is_running) { + session_start(); +} Is this neccessary? 'User ID '.$cron_uid.' does not exist' ------------------------------------------------------------------------ Sun, 13 Mar 2005 20:11:09 +0000 : killes(a)www.drop.org 'User ID '.$cron_uid.' does not exist' < == needs to be made translatable. Setting to active. ------------------------------------------------------------------------ Mon, 14 Mar 2005 05:56:17 +0000 : Chris Johnson Attachment: http://drupal.org/files/issues/cron_uid3.patch (3.29 KB) Ooops. Sorry I missed that text that needed translating. New patch attached with that bit corrected. Yes, I think the if (!$cron_is_running) code is needed to keep the cron user from creating many unneeded sessions. ------------------------------------------------------------------------ Mon, 14 Mar 2005 09:48:51 +0000 : stefan nagtegaal I am all for the idea of introducing a special id for running the cronjobs, but the approach you took here is far from perfect imo.. After looking at your patch, there are several questions and remarks that came up to my mind: I agree with Gerhard that the $cron_is_running is probably unneeded, I think it's better todo: <?php if (!variable_get('cron_busy', true)) { session_start(); } ?> I think that this is much to technical. Severa newbies will probable ask what a user id is, or what cron jobs are.. From both usability and userfriendlyness this patch is a big -1 from me.. I would suggest todo the following: - Make a new user in the db called 'Cron' with a user id -1, or something like that.. That way you don't have to add extra options to the administration, and the way of implementing should be much cleaner. Also does it improves usability... What do you think? ------------------------------------------------------------------------ Mon, 14 Mar 2005 13:11:44 +0000 : Anonymous Hi, The functionality is great, but your interface is not. IMO you should just choose good defaults and leave it with that. Why would users want to choose "UIDs" ofr "cron Jobs". Joe user should not have to think about UIDs and Crons. IT should Just Work[tm]. I know there are lots of places in Drupal where this is not the case either, but we are working on that. So when we introduce new functionality we should do it good at once. Why not go for Stefans suggestion of choosing a cron UID and jsut leave it like that. UIDs are not really used anyway, so you can choose just any UID (just the next one available, most often will be '2'). Bèr ------------------------------------------------------------------------ Mon, 14 Mar 2005 13:45:54 +0000 : javanaut "Make a new user in the db called 'Cron' with a user id -1" Just on a technical note, the database type for uids is "unsigned" and -1 won't work. ------------------------------------------------------------------------ Mon, 14 Mar 2005 19:08:48 +0000 : killes(a)www.drop.org @Chris: I am pretty sue that this patch will not be usefull if the cron user does not get a session. Drupal relies on the session to authenticate the user. If you are worried about too many sessions, you should make sure the session is clossed at the end of the run. his could be done in cron.php. @Stefan, Ber: I think we should introduce just a standard user and assign the "authneticated user" role to him. The set the variable to that uid so we can check which user it is. The problem is that we cannot create the user from database.mysql beacuse the next uid would be No. 1 which is reserved. So we would need to create the user on the first visit to the admin/settings page. We could also use drupal_set_message to inform the admin of this. We should also extent the relevant help text. While we are at it we could make the anon user use the db tabe for its name too and not a variable. ------------------------------------------------------------------------ Mon, 14 Mar 2005 20:33:00 +0000 : Chris Johnson Looks like we are sort of in a corner. Really the cron user should be in the database at install time, but we have to reserve uid 1 for the first user. Maybe there is another way to do that without having the first entry to admin create the cron user? That seems hackish to me. I agree that the anonymous user should also be completely coded into the database, and not a variable. The current methods causes anomolous behaviors. I, in fact, wrote several patches to do that a long time ago (http://drupal.org/node/5639) I looked at updating that patch a couple of months ago, but the new unregistered user comment capability where the user can enter his name made the code a lot more complicated. Maybe I will get motivated to try it again. If only my patch had been applied the first time! :-) ------------------------------------------------------------------------ Mon, 14 Mar 2005 21:21:12 +0000 : Olen How about creating a bunch of "reserved" users (like most linux distributions do)? uid 1 = root uid 2 = cron uid 3 - 8 = reserved for later use uid 9 = anonymous And then let the admin log in as root the first time. We still let him/her change the username of any of these users, so it should not cause to many problems for new installations. Older ones would either just have to live with it, or do a lot of 'UPDATE module_table SET uid=X WHERE uid=2'-queries. ------------------------------------------------------------------------ Mon, 14 Mar 2005 22:33:32 +0000 : Bèr Kessels What about following the linux thing, and start counting at 500 for normal users? That way people will recognise it, and numbers do not matter anyway. I mean, there is really no difference between 4 and 500, other than thats its a different number. 500 makes just as much sense as 10, or 2 or 4. Bèr ------------------------------------------------------------------------ Tue, 15 Mar 2005 12:59:18 +0000 : Philippe The reason why I created the "ugly" user interface, and let the site admin choose a cron-user ID in the settings page is because of backward compatibility with existing installations. (guess what: my site IS an existing installation) Perhaps another option is to give special users a user ID close to 2147483647 (MAXINT). That way it won't interfere with existing users. ------------------------------------------------------------------------ Thu, 16 Jun 2005 23:12:05 +0000 : degerrit At the risk of being publicly humiliated, and in the spirit of "there's no such thing as a stupid question" ... what exactly is the (security?) issue with adding: $user = user_load(array('uid' => 1)); in cron.php? I have two sites with node_privacy_by_role (NPBR) enabled, and cron does not index all the pages as a consequence (except when it is given a user id with full access like above). It took me some puzzling to figure out why the sites seemed stuck at "11% of the site has been indexed" or something similar. Of course, now a non-authenticated user can see excerpts of "private" pages through the search, which could be a big deal. Anyway, being able to choose a search user would solve the problem with NPBR as well it seems. ------------------------------------------------------------------------ Fri, 12 Aug 2005 15:59:10 +0000 : robertDouglass Why do we need a user besides uid=0 for cron? Just because the project module writes anonymous on certain closed issues? If cron runs with uid=0 most of the time anyway, why don't we just hardcode that in instead of creating a new type of user?

1 0