February 2005 - Development - lists.drupal.org

[drupal-devel] [task] Clean up block module tables
by Dries 23 Feb '05

23 Feb '05

Project: Drupal Version: cvs Component: block.module Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: Dries Status: patch I guess this patch needs to be updated then? Dries Previous comments: ------------------------------------------------------------------------ December 9, 2004 - 23:07 : drumm Attachment: http://drupal.org/files/issues/block.module_2.diff (9.54 KB) * renamed 'boxes' to 'blocks_custom' * changed the 'bid' column of that table to 'delta' and removed auto increment in favor of the usual db_next_id * made associated changes in code to avoid 'box' and 'bid' * removed unique constriant on custom box titles (http://drupal.org/node/11724) * put the delete link for custom blocks back in, it was apparently lost in another update ------------------------------------------------------------------------ December 9, 2004 - 23:26 : jhriggs I think the only thing I would question is the renaming of bid. It seems inconsistent with the rest of drupal. We use nid, rid, uid, etc., etc. So, this should be a block ID, bid. ------------------------------------------------------------------------ December 10, 2004 - 00:05 : drumm Delta is the best name since this is what hook_block() calls it. Adding a bid (b as in block) to the blocks table is a good idea, but I wouldn't want to confuse things with a patch that is renaming bid in one context and adding bid in a related but different context. ------------------------------------------------------------------------ December 10, 2004 - 00:22 : Dries I prefer bid. I'd rather change delta to bid, instead of changing bid to delta. It is more consistent. Otherwise the patch looks good. ------------------------------------------------------------------------ December 10, 2004 - 00:43 : drumm Delta really isn't a good id. It isn't unique. The closest thing to a primary key is the blocks table is (module, delta). I think bid should be added as a real primary key on the blocks table. If there is a good word to use in place of delta that isn't bid, then that would be good to use. ------------------------------------------------------------------------ December 11, 2004 - 02:22 : drumm Updated patch which adds: * Added index to weight column in blocks table * Added primary key bid to blocks table * Replaced module/delta construct with bid where possible In additon to the previous: * renamed 'boxes' to 'blocks_custom' * changed the 'bid' column of that table to 'delta' and removed auto increment in favor of the usual db_next_id * made associated changes in code to avoid 'box' and 'bid' * removed unique constriant on custom box titles (http://drupal.org/node/11724) * put the delete link for custom blocks back in, it was apparently lost in another update ------------------------------------------------------------------------ December 11, 2004 - 02:23 : drumm Attachment: http://drupal.org/files/issues/block.module_3.diff (20.41 KB) And the patch too ------------------------------------------------------------------------ December 12, 2004 - 10:58 : Dries The upgrade path gave me problems. I got a fatal error: <?php $ret[] = update_sql('ALTER TABLE {blocks_custom} DROP INDEX title'); ?> The index did not exist on my setup. Weird. I got several hundred warnings about this line: <?php foreach ($blocks as $delta => $block) { ?> It's somewhat tricky, because you can't execute that update twice. My user and block tables are pretty unusable now. :) (Is the block->info field still being used? It looks quite redundant to me.) ------------------------------------------------------------------------ December 12, 2004 - 11:52 : Dries I ran this update on a copy of the drupal.org database and it seems to work. The reason my previous attempt failed, is because of the way error reporting was configured. My copy of the drupal.org database doesn't have an index on 'blocks_custom.title' either, but it did not result in a fatal error nor did it cause the upgrade script to halt. Warning are suppressed so they are simply not shown. In short, depending on how error reporting is configured, the upgrade path might or might not work. If it doesn't work, you're screwed because you won't be able to run the upgrade script again. At least, not without intimate knowledge of Drupal's internals. ------------------------------------------------------------------------ February 22, 2005 - 12:57 : TDobes It should be noted that the patch here contains a fix for a CRITICAL bug in block.module: At the moment, custom blocks cannot be deleted. A patch containing the fix alone (without other improvements) is located in another issue, which was marked as a duplicate [1] quite a while ago. Since we're now in a feature freeze, perhaps I should reopen the other issue so the deletion fix alone can be committed? [1] http://drupal.org/node/14625 ------------------------------------------------------------------------ February 22, 2005 - 13:06 : TDobes Oops... looks like Steven already committed a fix for the problem I noted... it just hasn't shown up in the CVS logs yet. Sorry for the noise. -- View: http://drupal.org/node/14158 Edit: http://drupal.org/project/comments/add/14158

1 0

[drupal-devel] [bug] node_validate does not respect group editing
by tangent 23 Feb '05

23 Feb '05

Project: Drupal Version: cvs Component: node.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: mathias Updated by: tangent Status: patch As requested in this issue [1], it may be desirable for users with the permission to do so to change the owner of a node. [1] http://drupal.org/node/17267 tangent Previous comments: ------------------------------------------------------------------------ September 25, 2004 - 13:23 : mathias With the new node-level access permissions, it is entirely possible for users in the same role, or having the same taxonomy term to edit each other's nodes. However when this happens, node_validate will transfer ownership of the node to the user who last edited it. I think this behavior should be changed so that original authorship is always maintained unless specifically transferred. The problem lies in node_validate, here: $node->uid = $user->uid ? $user->uid : 0; Since an alteration such as this could introduce an exploit, I'm wondering what other's feel would be the best solution? I was working on a role-based editing permissions module (based on JonBob's nodeperm_role.module) where the author of a node controls which groups can view/edit their post. ------------------------------------------------------------------------ September 25, 2004 - 16:34 : mathias Attachment: http://drupal.org/files/issues/node_perm.patch (655 bytes) Here is a proposed patch, which would then allow node authors to choose which users could view/edit their post. ------------------------------------------------------------------------ December 21, 2004 - 11:42 : moshe weitzman seems simple enough to me. we ought to protect against unintentially changing the author, right? +1 ------------------------------------------------------------------------ December 27, 2004 - 04:16 : Dries Hopefully, this will become easier/clear as soon the revisions patch hit CVS. Let's revisit this soon. -- View: http://drupal.org/node/11071 Edit: http://drupal.org/project/comments/add/11071

1 0

[drupal-devel] [bug] file_check_location is not working on Windows 2000
by dan90 23 Feb '05

23 Feb '05

Project: Drupal -Version: cvs +Version: 4.5.2 Component: file system Category: bug reports Priority: critical Assigned to: Anonymous Reported by: ccourtne Updated by: dan90(a)drupal.org Status: patch This also fixed my file upload problems with some nonstandard paths under Linux/apache, php 4.3.4 drupal 4.5.2. maybe we should remove mention of windows 2000? dan90(a)drupal.org Previous comments: ------------------------------------------------------------------------ September 16, 2004 - 13:46 : ccourtne Currently file_check_location is always returning false when passed a file as the $source path. If a directory is passed in $soruce file_check_location works. I've tracked this down to realpath returning an empty string when passed a relative path which points to a file. This probably goes unnoticed if you are using the file.api's to construct relative paths to the druapl file directory since check_location failing just causes the durpal file directory to get added which is what you wanted in the first place. Replacing this line $source = realpath(dirname($source)); with this $source = realpath(dirname($source)); fixes it on my windows box using PHP 4.3.8. I don't have things set up to do patches on my windows boxes here at work yet. I'll submit a formal patch tonight. ------------------------------------------------------------------------ September 16, 2004 - 13:47 : ccourtne Dag nabit. Replace $source = realpath($source); with $source = realpath(dirname($source)); Craig ------------------------------------------------------------------------ September 16, 2004 - 13:56 : Anonymous Attachment: http://drupal.org/files/issues/file-inc-file_check_location.patch (639 bytes) The sooner there are patches, the better it is... I tested this approach on my server and it works. Steef ------------------------------------------------------------------------ September 17, 2004 - 01:34 : ccourtne Attachment: http://drupal.org/files/issues/checklocation.patch (666 bytes) Here is the patch. -- View: http://drupal.org/node/10885 Edit: http://drupal.org/project/comments/add/10885

1 0

[drupal-devel] [bug] Putting a .theme file into themes/ makes all themes not working
by chx 23 Feb '05

23 Feb '05

Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: critical Assigned to: chx Reported by: chx Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/themefix_1.patch (712 bytes) Message was wrong. chx Previous comments: ------------------------------------------------------------------------ February 7, 2005 - 08:13 : chx Try putting a .theme file into themes/ without any subdir. Enable it (maybe just saving theme settings is enough), Nothing will work after this. To restore your site: remove the offending theme, issue an SQL: DELETE FROM {system} WHERE type='theme' and save theme settings again. ------------------------------------------------------------------------ February 7, 2005 - 08:14 : chx effects HEAD, too. ------------------------------------------------------------------------ February 7, 2005 - 13:33 : chx Attachment: http://drupal.org/files/issues/themesfix.patch (542 bytes) Here is a patch which fixes the problem. However, I do not have the feeling that this is "the" solution. Feel free to come up with something better. ------------------------------------------------------------------------ February 7, 2005 - 13:35 : chx Oops forgot to set to CVS, 'cos the fix is against HEAD. Not that 4.5.2 does not need a patch... ------------------------------------------------------------------------ February 7, 2005 - 22:15 : Anonymous While this patch would allow a theme to be placed directly in the "themes" directory, it disables the ability for that theme to use a "style.css" file, which is inconsistent. I would prefer a patch that enforces the criteria that themes MUST be placed in a SUBDIRECTORY of "themes" -- themes placed directly in the "themes" folder should be ignored... maybe we could send the admin a drupal_set_message that would tell them about the problem when visiting admin/themes too. ------------------------------------------------------------------------ February 23, 2005 - 00:07 : chx Attachment: http://drupal.org/files/issues/themefix.patch (693 bytes) Anonymous is right. I have made a better patch. Please commit, this is very important IMHO. -- View: http://drupal.org/node/16914 Edit: http://drupal.org/project/comments/add/16914

1 0

[drupal-devel] [bug] Putting a .theme file into themes/ makes all themes not working
by chx 23 Feb '05

23 Feb '05

Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: critical Assigned to: chx Reported by: chx Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/themefix.patch (693 bytes) Anonymous is right. I have made a better patch. Please commit, this is very important IMHO. chx Previous comments: ------------------------------------------------------------------------ February 7, 2005 - 08:13 : chx Try putting a .theme file into themes/ without any subdir. Enable it (maybe just saving theme settings is enough), Nothing will work after this. To restore your site: remove the offending theme, issue an SQL: DELETE FROM {system} WHERE type='theme' and save theme settings again. ------------------------------------------------------------------------ February 7, 2005 - 08:14 : chx effects HEAD, too. ------------------------------------------------------------------------ February 7, 2005 - 13:33 : chx Attachment: http://drupal.org/files/issues/themesfix.patch (542 bytes) Here is a patch which fixes the problem. However, I do not have the feeling that this is "the" solution. Feel free to come up with something better. ------------------------------------------------------------------------ February 7, 2005 - 13:35 : chx Oops forgot to set to CVS, 'cos the fix is against HEAD. Not that 4.5.2 does not need a patch... ------------------------------------------------------------------------ February 7, 2005 - 22:15 : Anonymous While this patch would allow a theme to be placed directly in the "themes" directory, it disables the ability for that theme to use a "style.css" file, which is inconsistent. I would prefer a patch that enforces the criteria that themes MUST be placed in a SUBDIRECTORY of "themes" -- themes placed directly in the "themes" folder should be ignored... maybe we could send the admin a drupal_set_message that would tell them about the problem when visiting admin/themes too. -- View: http://drupal.org/node/16914 Edit: http://drupal.org/project/comments/add/16914

1 0

[drupal-devel] [feature] Search module should have a simple search block
by tangent 22 Feb '05

22 Feb '05

Project: Drupal Version: cvs Component: search.module Category: feature requests Priority: normal Assigned to: erikhopp Reported by: [viz] Updated by: tangent -Status: active +Status: patch Attachment: http://drupal.org/files/issues/search_block_1.patch (1.61 KB) The attached patch adds a hook_block and uses search_form to add the form to the block. I've extended the search_form function so that modules can alter the text prompt as having the prompt visible in a block looks bad. I created this patch prior to searching for this issue to apologies to those who have put in work on this. tangent Previous comments: ------------------------------------------------------------------------ November 23, 2002 - 03:34 : [viz] Hey just one quick question/feature request. There may be an obvious way to do this, but I am stumped. Documentation only lists a way to put all "left" blocks on the left, and "right" blocks on the right (I already modded xsilver to work for my site's style) - but what if I have a split header for my page (graphics on left, dynamic content on the right) and I want to offer the search block on the right half if a user is logged in, and a login block there if they aren't? I sniped the code out for a search and that appears there now ( linux.trustdevelopment.com ) but I don't want it to appear for users that aren't logged in, since I want searching to be one of the "premium services" that a person would want to register an account for... Thanks... Josh ------------------------------------------------------------------------ March 12, 2003 - 01:06 : al You need to create a search.module that puts a search box in a block, using the already existing search permission to determine whether users can access it (i.e. it gets displayed) or not. I'm moving this to a contribs feature request. Maybe someone will want to implement it for you. It would be really trivial to do... ------------------------------------------------------------------------ May 29, 2003 - 20:46 : al After some thought, I've decided that having the search module able to put a simple search text box in a block with a "search" button would be a useful thing to do. I'm therefore reassigning the project, etc. for this and changing the summary to reflect this. This would be really very easy to implement. Anyone? ------------------------------------------------------------------------ May 29, 2003 - 23:45 : erikhopp hey. ok, so i don't really know php and i haven't really played with the drupal core too much, so maybe it's not right, but i think i have created a block for the search module. it works on my site (i think). the only problem is that i just cannot figure out how to change the size of the input box. it is set at 50 for the search page, but i can't seem to figure out where that value is set (and i feel like i have looked everywhere). so if someone can change that, i think this feature request is done. oh - and i still have not spent the time to figure out how to add stuff via cvs so the search module is here: http://mediamutiny.org/search.module and i updated the $id. i'm not sure if i am supposed to do that or not. i hope this helps... erik. ------------------------------------------------------------------------ May 30, 2003 - 00:46 : al Your module looks like it returns the search results inside the block, which isn't quite what's wanted. This is actually a bit of a mess - the search_form function should be able to do what we want, but you can't actually use it due to various issues (like the width). I note that the xtemplate theme uses its own search form. I'm sending a patch to the mailing list that does the same thing. BTW, the $Id: $ field in files is generated automatically by the CVS server. Don't alter it yourself - it'll make us all terribly confused... ------------------------------------------------------------------------ June 19, 2003 - 10:15 : al A patch to implement this has been sitting in my sandbox for ages. Dries - is there something wrong with the patch? Or do you just not think this to be a valid feature request? Should it be done with a completely seperate module? Or be theme-specific? ------------------------------------------------------------------------ September 8, 2003 - 17:55 : erikhopp so what is the status of this search block patch? i'd like to see it added if it works ok. erik. ------------------------------------------------------------------------ October 28, 2003 - 00:17 : erikhopp one more attempt to get this committed... the patch is still in al's sandbox... i have it enabled on one of my sites and it works great... erik. ------------------------------------------------------------------------ April 20, 2004 - 18:30 : kepol I need a search block so I'm writing one right now, but maybe you guys have one floating around already??? If so, where is it? :) Thanks, Kristen ------------------------------------------------------------------------ April 20, 2004 - 18:51 : gordon Actually when ever I need a search block I actually just create a custom block and I have never had a problem, see below. <form action="index.php?q=search" method="post"> <br /> <input type="text" class="form-text" size="15" name="keys" /> <input type="submit" class="form-submit" value="Search" /> </form> ------------------------------------------------------------------------ June 18, 2004 - 17:02 : erikhopp Attachment: http://drupal.org/files/issues/search_block.patch (1.02 KB) here is a patch that adds a search block. works with 4.4.x and should work with HEAD too. ------------------------------------------------------------------------ June 18, 2004 - 19:57 : moshe weitzman In order to mark an issue as 'patch', please submit a patch against HEAD and test it. Moving back to Active. Also, consider using the form_x functions instead of hard coding HTML into the block. ------------------------------------------------------------------------ July 10, 2004 - 16:58 : leafish_paul Apologies for not including a patch here, but for anyone who's interested, here's my amendment to erikhopp's patch (using drupal's form functions as suggested): <?php function search_block_view() { $output .= form_textfield( $title='Search in this site', $name='keys', $value='', $size=15, $maxlength=128 ); $output .= form_submit( $value="Go" ); return "<div class=\"search-block\">". form($output, "post", url("search")) ."</div>"; } ?> ------------------------------------------------------------------------ July 13, 2004 - 16:10 : erikhopp Attachment: http://drupal.org/files/issues/search_block_0.patch (764 bytes) ok. thanks leafish_paul. here is a patch that applies to HEAD. ------------------------------------------------------------------------ July 14, 2004 - 15:05 : erikhopp forgot to set to patch... ------------------------------------------------------------------------ July 14, 2004 - 15:49 : Dries The patch looks incomplete to me. You have to implement 'search_block()' for this work ... ------------------------------------------------------------------------ February 4, 2005 - 19:31 : Steven The patch is incomplete and uses borky code (the whole $var=value thing in the search block function is theoretically correct, but practically stupid). Perhaps the current search_form function should simply be modified with a width parameter so it can be used in the block too? -- View: http://drupal.org/node/820 Edit: http://drupal.org/project/comments/add/820

1 0

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

22 Feb '05

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

1 0

[drupal-devel] [task] Clean up block module tables
by TDobes 22 Feb '05

22 Feb '05

Project: Drupal Version: cvs Component: block.module Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: TDobes Status: patch Oops... looks like Steven already committed a fix for the problem I noted... it just hasn't shown up in the CVS logs yet. Sorry for the noise. TDobes Previous comments: ------------------------------------------------------------------------ December 9, 2004 - 15:07 : drumm Attachment: http://drupal.org/files/issues/block.module_2.diff (9.54 KB) * renamed 'boxes' to 'blocks_custom' * changed the 'bid' column of that table to 'delta' and removed auto increment in favor of the usual db_next_id * made associated changes in code to avoid 'box' and 'bid' * removed unique constriant on custom box titles (http://drupal.org/node/11724) * put the delete link for custom blocks back in, it was apparently lost in another update ------------------------------------------------------------------------ December 9, 2004 - 15:26 : jhriggs I think the only thing I would question is the renaming of bid. It seems inconsistent with the rest of drupal. We use nid, rid, uid, etc., etc. So, this should be a block ID, bid. ------------------------------------------------------------------------ December 9, 2004 - 16:05 : drumm Delta is the best name since this is what hook_block() calls it. Adding a bid (b as in block) to the blocks table is a good idea, but I wouldn't want to confuse things with a patch that is renaming bid in one context and adding bid in a related but different context. ------------------------------------------------------------------------ December 9, 2004 - 16:22 : Dries I prefer bid. I'd rather change delta to bid, instead of changing bid to delta. It is more consistent. Otherwise the patch looks good. ------------------------------------------------------------------------ December 9, 2004 - 16:43 : drumm Delta really isn't a good id. It isn't unique. The closest thing to a primary key is the blocks table is (module, delta). I think bid should be added as a real primary key on the blocks table. If there is a good word to use in place of delta that isn't bid, then that would be good to use. ------------------------------------------------------------------------ December 10, 2004 - 18:22 : drumm Updated patch which adds: * Added index to weight column in blocks table * Added primary key bid to blocks table * Replaced module/delta construct with bid where possible In additon to the previous: * renamed 'boxes' to 'blocks_custom' * changed the 'bid' column of that table to 'delta' and removed auto increment in favor of the usual db_next_id * made associated changes in code to avoid 'box' and 'bid' * removed unique constriant on custom box titles (http://drupal.org/node/11724) * put the delete link for custom blocks back in, it was apparently lost in another update ------------------------------------------------------------------------ December 10, 2004 - 18:23 : drumm Attachment: http://drupal.org/files/issues/block.module_3.diff (20.41 KB) And the patch too ------------------------------------------------------------------------ December 12, 2004 - 02:58 : Dries The upgrade path gave me problems. I got a fatal error: <?php $ret[] = update_sql('ALTER TABLE {blocks_custom} DROP INDEX title'); ?> The index did not exist on my setup. Weird. I got several hundred warnings about this line: <?php foreach ($blocks as $delta => $block) { ?> It's somewhat tricky, because you can't execute that update twice. My user and block tables are pretty unusable now. :) (Is the block->info field still being used? It looks quite redundant to me.) ------------------------------------------------------------------------ December 12, 2004 - 03:52 : Dries I ran this update on a copy of the drupal.org database and it seems to work. The reason my previous attempt failed, is because of the way error reporting was configured. My copy of the drupal.org database doesn't have an index on 'blocks_custom.title' either, but it did not result in a fatal error nor did it cause the upgrade script to halt. Warning are suppressed so they are simply not shown. In short, depending on how error reporting is configured, the upgrade path might or might not work. If it doesn't work, you're screwed because you won't be able to run the upgrade script again. At least, not without intimate knowledge of Drupal's internals. ------------------------------------------------------------------------ February 22, 2005 - 04:57 : TDobes It should be noted that the patch here contains a fix for a CRITICAL bug in block.module: At the moment, custom blocks cannot be deleted. A patch containing the fix alone (without other improvements) is located in another issue, which was marked as a duplicate [1] quite a while ago. Since we're now in a feature freeze, perhaps I should reopen the other issue so the deletion fix alone can be committed? [1] http://drupal.org/node/14625 -- View: http://drupal.org/node/14158 Edit: http://drupal.org/project/comments/add/14158

1 0

[drupal-devel] Spread Fire Fox theme in CVS
by K B 22 Feb '05

22 Feb '05

A working version of the Spread Fire Fox theme by Chris Messina has been committed to the repository. I ported the theme from CivicSpace (4.4?) to 4.5. and CVS. Please check the announcement here http://drupal.org/node/17717

1 0

[drupal-devel] Marketplace forum?
by K B 22 Feb '05

22 Feb '05

Regarding this post: http://drupal.org/node/17697 Is it time to setup a "marketplace" forum for these kinds of requests?

4 3