- Development - lists.drupal.org

[drupal-devel] [bug] xtemplate mission does not work with an alias
by Anonymous 27 Jan '05

27 Jan '05

Project: Drupal Version: 4.4.1 Component: theme system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: kbahey Updated by: Anonymous -Status: duplicate +Status: patch This was not fixed by the patch to the other issue. What needs to happen here is: variable_get("site_frontpage", "node") needs to become drupal_get_normal_path(variable_get("site_frontpage", "node")) I'm not on a workstation where I can post a real patch at the moment, but this is a very simple change, so I'm changing the status. Anonymous Previous comments: ------------------------------------------------------------------------ May 11, 2004 - 02:18 : kbahey When using xtemplate, and setting "xtemplate_mission" to a value (under "Message on front page" on /admin/system/themes/xtemplate), the mission does not appear if the /admin/system "Default front page" is set to a url alias. So, if the front page should be set to "node/view/8" for example, and there is a url alias for that node (say: "index.html"), then the mission will not show up. The culprit code is in xtemplate.theme [?php // only parse the mission block if we are on the frontpage ... if ($_GET["q"] == variable_get("site_frontpage", "node") && ($mission = variable_get("xtemplate_mission", "edit mission"))) { $xtemplate->template->assign("mission", $mission); $xtemplate->template->parse("header.mission"); } ?] _GET["q"] does not contain the alias ("index.html"), but the original node ("node/view/8") Of course, the workaround is easy (put the node and not the alias as the front page) However, it is best to be consistent across the entire system, and treat the alias exactly as the original node. ------------------------------------------------------------------------ July 27, 2004 - 16:15 : JonBob Folded into http://drupal.org/node/view/9477. -- View: http://drupal.org/node/7685 Edit: http://drupal.org/project/comments/add/7685

1 0

[drupal-devel] [feature] Proposed change to menu.inc that would allow modules to create custom menus
by JonBob 27 Jan '05

27 Jan '05

Project: Drupal Version: cvs Component: menu system Category: feature requests Priority: normal Assigned to: andremolnar Reported by: andremolnar Updated by: JonBob Status: patch I'd prefer a solution that allowed modules to declare such menu items using the menu hook, but haven't been able to come up with such a solution. I'm okay with this approach until such time as someone has a brilliant idea to make the menu hook more powerful. JonBob Previous comments: ------------------------------------------------------------------------ January 6, 2005 - 02:13 : andremolnar Attachment: http://drupal.org/files/issues/menu_inc_2.patch (2.65 KB) See http://drupal.org/node/15153 and http://drupal.org/node/14120 for discussion on this proposed change. Patch is included here for further discussion (with the changes suggested by Dries). A patch is forthcoming for topics discussed in http://drupal.org/node/14120 which would make use of this patch. Status to be changed to patch later if there are no objections following discussion. andre ------------------------------------------------------------------------ January 8, 2005 - 16:16 : andremolnar Patch making use of this is now added to http://drupal.org/node/14120. andre ------------------------------------------------------------------------ January 10, 2005 - 02:29 : andremolnar Just realising that the rules for setting status to patch would allow this to be patch since its technically ready to go. (and it might actually get noticed if its on the dev list and patch queue). andre ------------------------------------------------------------------------ January 11, 2005 - 01:47 : Dries I'm OK with this patch but won't commit it yet until the book module patch is ready. ------------------------------------------------------------------------ January 19, 2005 - 21:07 : andremolnar I've uploaded the latest version of the patch to book. http://drupal.org/node/14120 andre -- View: http://drupal.org/node/15198 Edit: http://drupal.org/project/comments/add/15198

1 0

[drupal-devel] [bug] Changing roles for a user requires menu cache invalidate
by JonBob 27 Jan '05

27 Jan '05

Project: Drupal Version: cvs Component: user.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: moshe weitzman Updated by: JonBob -Status: active +Status: patch Attachment: http://drupal.org/files/issues/menu_18.patch (1.51 KB) Fixed by adding ability to invalidate cache only for one user. JonBob Previous comments: ------------------------------------------------------------------------ December 31, 2004 - 13:46 : moshe weitzman When you remove a user from a given role, the effect is often not seen for a long time since the user's menu remains in cache. we should invaidate the menu cache for a user whenh is roles change (or anytime the $user is saved?). This same bug is apparent when you grant new roles to a user. -- View: http://drupal.org/node/14977 Edit: http://drupal.org/project/comments/add/14977

1 0

[drupal-devel] [feature] Support curl for cron job execution
by budda 27 Jan '05

27 Jan '05

Project: Drupal Version: 4.5.2 Component: other Category: feature requests Priority: normal Assigned to: Anonymous Reported by: budda Updated by: budda Status: patch Attachment: http://drupal.org/files/issues/cron-curl.sh (66 bytes) Attached a "missing" script to allow the use of curl as a way to kick off cron.php My shared hosting does not support lynx or wget. The attached script has been working find on www.spamnews.co.uk for a while. budda -- View: http://drupal.org/node/16298 Edit: http://drupal.org/project/comments/add/16298

1 0

[drupal-devel] [feature] Forum admin screen updates
by Dries 27 Jan '05

27 Jan '05

Project: Drupal Version: cvs Component: forum.module Category: feature requests Priority: normal Assigned to: ccourtne Reported by: ccourtne Updated by: Dries Status: patch I'm afraid it does no longer apply. It's probably fairly easy to update though. Dries Previous comments: ------------------------------------------------------------------------ August 29, 2004 - 08:50 : ccourtne Attachment: http://drupal.org/files/issues/forumadmin.patch (18.5 KB) Added admin screens to maintain forums instead of forcing the user to add a taxonomy. In addition the patch hides the forum taxonomy from the categories screen since the forum module does not support all features of taxonomies. The patch still needs clean up work for comments and the delete function needs to remove forum topics when you remove a forum, but I wanted to get it out for review. This patch requires the latest forum speed up patch found here http://drupal.org/node/view/10308. Craig ------------------------------------------------------------------------ September 9, 2004 - 01:25 : ccourtne Attachment: http://drupal.org/files/issues/forumadmin2.patch (22.84 KB) New patch that properly deletes forum topics when the forum is deleted, has all code comments, and updates the help text. ------------------------------------------------------------------------ September 9, 2004 - 01:40 : ccourtne some screen shots of the admin screens http://homepage.mac.com/ccourtne/drupal/forum_add_forum.png http://homepage.mac.com/ccourtne/drupal/forum_add_forum.png http://homepage.mac.com/ccourtne/drupal/forum_add_container.png http://homepage.mac.com/ccourtne/drupal/forum_config.png ------------------------------------------------------------------------ September 9, 2004 - 01:42 : ccourtne One more screenshot. http://homepage.mac.com/ccourtne/drupal/forum_overview.png ------------------------------------------------------------------------ September 9, 2004 - 02:21 : ccourtne updating to patch status ------------------------------------------------------------------------ September 9, 2004 - 07:17 : Dries Looks good! ------------------------------------------------------------------------ September 15, 2004 - 07:08 : ccourtne Attachment: http://drupal.org/files/issues/forumadmin3.patch (22.77 KB) Screenshots have a new home. http://www.dawnsedge.com/ccourtne/drupal/forum_add_forum.png http://www.dawnsedge.com/ccourtne/drupal/forum_overview.png http://www.dawnsedge.com/ccourtne/drupal/forum_add_container.png http://www.dawnsedge.com/ccourtne/drupal/forum_config.png Also updated the patch for current CVS head. ------------------------------------------------------------------------ September 20, 2004 - 16:19 : walkah not only do i think this looks very good, but i also think image.module folks should maybe take notice :) (hint hint). oh, and weren't we gonna split taxonomy.module into taxonomy.inc + taxonomy.module at one point? ------------------------------------------------------------------------ September 21, 2004 - 20:47 : Dries I'm going to commit as soon CVS HEAD is opened up for development. In the mean time, keep refining this patch so other modules can learn from it. ------------------------------------------------------------------------ January 27, 2005 - 03:14 : Boris Mann Is this going to get committed? Talk about it on CS developers list. Also: can this separate multiple forums? Can it be extended to support multiple forums? -- View: http://drupal.org/node/10425 Edit: http://drupal.org/project/comments/add/10425

1 0

[drupal-devel] [feature] Block regions
by Kiev1.org 27 Jan '05

27 Jan '05

Project: Drupal Version: cvs Component: block.module Category: feature requests Priority: normal Assigned to: arnabdotorg Reported by: arnabdotorg Updated by: Kiev1.org Status: patch http: // kiev1.org/download/block_module.zip only it works while only with the usual themes, not using xtemplate - to use so (there an example of a theme inside archive) - on similarity as was $leftblocks = theme ("blocks", "left"); $rightblocks = theme ("blocks", "right"); but in an any kind $yyy=theme ("blocks", " anywhere 1 "); $zzz=theme ("blocks", " for example here "); $nnn=theme ("blocks", " and here it it is possible "); $mmm=theme ("blocks", " or here it it is possible "); Variables then to deduce where it is necessary to place in design group of blocks as they were deduced earlier only to an attribute at the left-on the right, now anywhere and as much as necessary, and the changed module block itself parse the file of a pattern for new such zones and in the administrator-panel of blocks shows those the dropping out list instead of a radiobud all that has found in the current theme and the block can be placed in an any zone instead of in left-right (in general it is an initial variant shall complete then that with theme engine worked) requre ALTER TABLE `blocks` CHANGE `region` `region` VARCHAR( 255 ) DEFAULT '0' NOT NULL Kiev1.org Previous comments: ------------------------------------------------------------------------ November 16, 2004 - 08:17 : arnabdotorg Attachment: http://drupal.org/files/issues/regions.png (6.46 KB) (I don't know if this point has been brought up before, so writing this...) A Thought: Why do we have just "left" and "right" areas for blocks? Ideally, regions should be configurable. This way we can have many possiblities, like header blocks, footer blocks, etc. imho, This is a very important feature for any good themeable CMS. The design pattern is: "One Node area, to show the current document; and many side block regions to show assosciated data" ------------------------------------------------------------------------ November 16, 2004 - 11:04 : arnabdotorg Has been talked of before, posting links for reference: http://drupal.org/node/2790 http://drupal.org/node/10653 ------------------------------------------------------------------------ November 16, 2004 - 16:38 : arnabdotorg Attachment: http://drupal.org/files/issues/patch_2.txt (2.56 KB) Patch to latest block.module in CVS; changes the binary {left, right} regions to {left, right, [custom....]} regions. Any theme can now use theme_blocks("XXXX") where XXXX is any region set up in the Blocks admin menu; not just left, or right. Current themes with {left,right} will be unaffected. Demo site: http://arnab.drupaldevs.org/ [authenticated user has block_admin perms] Move the blocks around "apples" "mangoes" and "grapes"; which are defined as regions in the theme (colored areas). shuffle... ------------------------------------------------------------------------ November 16, 2004 - 17:07 : Anonymous This is potentially a very useful patch. It should be noted that this would only be added to the CVS HEAD branch, not to 4.5, as 4.5 is in a bugfix-only stage. Also, would you mind resubmitting your patch using the unified diff format? See these instructions for submitting patches [1]. [1] http://drupal.org/patch ------------------------------------------------------------------------ November 16, 2004 - 17:22 : arnabdotorg Attachment: http://drupal.org/files/issues/patch_3.txt (4.01 KB) - diff format fixed, attached. - full code: http://arnab.drupaldevs.org/files/block.module.phps ------------------------------------------------------------------------ November 16, 2004 - 17:36 : stefan nagtegaal Wouldm't it be nicer if the theme itself spits out the block_regions which it supports? So, in your theme: Which will be parsed through the theme settings where we can select which blocks to display in which region.. Imo this is going to the right direction, but isn't really the way it should be yet.. What happens if I had a theme anbled which has 4 regions, and I switch to a theme which only supports one? Stefan ------------------------------------------------------------------------ November 16, 2004 - 18:27 : tostinni Can't we log on your demo site using drupal ID ? ------------------------------------------------------------------------ November 16, 2004 - 18:31 : arnabdotorg 1. All themes support either {left,right} or {all} 2. If you notice that a theme doesn't support a region, either rename the region, or create that region and move the block to that region. Dependency handling is imho overkill for the job at hand. In other words, you can always move blocks back to left / right. ------------------------------------------------------------------------ November 16, 2004 - 19:03 : drumm I think this feature is greatly needed. Allowing more block regions could clear away a bunch of the clutter in the theme configuration pages. However, providing a UI which won't work unless you have intimate knowlege of your theme (who would guess 'mangoes' is a region) is quite bad. I think something closer to the suggestion in #5 should be used. The edge cases, changing themes and allowing users to pick their own theme, provides some problems. I don't have any good ideas for that. ------------------------------------------------------------------------ November 17, 2004 - 03:37 : arnabdotorg Attachment: http://drupal.org/files/issues/block.module.patch (4.33 KB) Incorporated ideas from #5; a theme can now have the function: If this function is defined, we check if this list is identical to the current list of regions setup. If it is not, we issue a critical warning to the user that things are amiss. This is done both during setting up the regions, and selecting a theme. If the themes_block_regions is not defined, nothing happens. Files affected: block.module system.module ------------------------------------------------------------------------ November 17, 2004 - 03:38 : arnabdotorg Attachment: http://drupal.org/files/issues/system.module_0.patch (811 bytes) ------------------------------------------------------------------------ November 17, 2004 - 23:17 : Goba OK, but what if a user changes the theme? A site can have multiple themes... You only check for the theme set when the admin sets the block regions... Also none of the themes should define functions starting with theme_, as this prefix is reserved for Drupal built in theme functions. Themes define theme functions with their own prefix. BTW I have also spotted a code style problem. We are not using string concatenation with SQL parameters, but the %s notation for strings and %d for numbers. Look for the db_query() implementation if you need more info. ------------------------------------------------------------------------ November 18, 2004 - 08:55 : Dries I agree that the theme should control the available regions (cfr. comment #5). As different themes can co-exist, the block positions should become configurable per-theme. ------------------------------------------------------------------------ November 18, 2004 - 14:08 : Bèr Kessels Attachment: http://drupal.org/files/issues/canvas-regions.png (15.48 KB) I love this idea of regions. And to add my 2C I investigated the existing themes, and my sites to find a genral set of regions. I placed them in a schetch. I also suggest to folow the naming convention in that scetch, since those names are already most common in Drupal themes. ------------------------------------------------------------------------ November 18, 2004 - 14:17 : Bèr Kessels Pressed. submit too early Sorry. I still had some additional comments: canvas = body. canvas.head.primary and canvas.head.secondary, could and should be merged into one, but thats still a project in preperation. It will then become canvas.head.navigation canvas.main.mission is a bit a silly name, but we use it currently a lot. IMO, it could better be renamed to canvas.main.pre. It can contain (error) messages, mission, etc. We should refrain from the words "node" since there is far more content than nodes.Hence I named it canvas.main.content. Maybe, when future dictates it, we could split up the canvas.main.content into four regions, canvas.main.content.NE canvas.main.content.SE canvas.main.content.SW canvas.main.content.NW. That would allow us to create more dynamical layouts, often used by portals on fromt page. (E.gG to show the headline picture top-left in content region. ------------------------------------------------------------------------ November 18, 2004 - 15:53 : arnabdotorg dries: you're right, Stefan and I were discussing the same thing. The "regions" configuration needs to be refactored out of block admin and moved into the theme configuration; regions configurable on a per theme basis. This also makes sense; region placement is a presentation issue and should be separated from content. However, I don't think the current DB schema for blocks will support this, options: 1) add "theme" attribute to {blocks} and store themeid [bad design, unnormalized data] 2) add "regions"(themeid, blockid, regionid) table for solely this feature. [normalized version of (1), but too much?] 3) store this in {system} somehow. goba: thanks for pointing those out, will make changes in next patch. berkes: Thanks for the sketch, helpful. Something like this can find a good place in the Theme developer's(designer's?) handbook, under "region naming guidelines"; or something like that. Note that I say "guidelines"; I don't think we should restrict designers to conform to any presentation metaphors. (not even {left, right}! :) ) However, such kind of guidelines can be coded into drupal for "intelligently" allocating regions for blocks - i.e. if we already have block->region configuration for a theme, it can be reused to create initial settings for a newly installed theme. Todo list: Decide on ... 1) how to store per-theme blockregion info. 2) function name to give for region discovery in place of themes_block_regions() Some layouts to browse: http://www.terminus1525.ca/ http://k10k.net/ http://www.stylegala.com/ http://www.iht.com/pages/index.html ------------------------------------------------------------------------ November 18, 2004 - 17:09 : Bèr Kessels More on 5the "naming guidelines". Of course they should be "guidelines" and not laws! But... Users are dumb. Users know nothing, And mostly: users should not need to know anything. Thats why I proposed the naming guidelines, and chopped up everything in regions. I would really, really like to see these regions to be the default regions. We should always try to deliver a ready-out-of-the-box working Drupal. We should really try to have everything working, by default. And try to make everything working, also without need to configure ofr even access the theme configuration. In other words: There must be a default set of regions (similar to what we have now, with only left and right) preferably (by me) the regions I named in my png. This set of regiosn should be default, and available in the default (core) themes. Bèr ------------------------------------------------------------------------ November 18, 2004 - 20:56 : Steven I'd like to point out that ber's sketch is just one possibility. Even the Drupal.org theme does not follow it, as the mission is between the main content and the header, and not flanked by the sidebars. We should avoid making structure part of the naming. ------------------------------------------------------------------------ November 18, 2004 - 23:38 : FactoryJoe(a)civicspacelabs.org We should avoid making structure part of the naming. This is key. In my latest theme (Democratica), I've changed the typical "heading" and "footer" IDs to be called "branding" and "notices" since that's what they're typically used for, rather than where they are to be positioned. I see a need for some leadership in determining both what regions should come standard with each theme, how to name the regions geographically as opposed to content use, and whether the control should lie in the CSS or in the theme admin interface (for instance, if I, as the theme designer want to force notices to be above the logo in my CSS). I think giving control to the users and admins is awesome and this idea is one that I planned to advocate in my as yet vapordoc Theme Proposal. I'd like to see where this discussion goes because while I think this functionality is important, I strongly feel like we need to develop an idea of what the ideal theme development/administration interface and workflow would look like rather than tossing on features that would do more to create bad site design rather than improve it or make it easy for non-designers. ------------------------------------------------------------------------ November 19, 2004 - 10:32 : stefan nagtegaal Something I don't get at the moment is why we want standardized names for the regions.. I think that it is better to make it regionname free, so it is for the theme administrator always clear where the blocks will appear.. In the situation we currently have, it is possible to set the region to 'left' and let the blocks appear on the right and the other way around, due to changing the theme. I think it is better to specify the regions inside any theme by doing: But, for another use of the site, a whole other block usage and approach can be used so the _regions() will look more like: Looking at the above examples we see that, with the use of free region-names, it is more clear to a third user (which downloaded the theme), for what purposes all these regions are ment to be used.. I'm not fully convinced about the following approach, but it is probably possible todo a module_invoke on the 'regions' hook or have a error returned when there is no overrided function is found for the standard theme_regions().. ------------------------------------------------------------------------ November 21, 2004 - 03:25 : FactoryJoe(a)civicspacelabs.org With all due respect, I think that this whole discussion is missing something... primarily a compatible goal and a purpose. On the one hand, we want to have more semantic markup and more flexibility from the layout engine (which, as of now, is simply a block configuration tool that allows you to choose "right" or "left" and order the blocks). Up until now, this functionality has sufficed, but when it comes to making more advanced layouts like k10k.net or downhillbattle.org, a much richer toolset is needed. So on the one hand, we have end-users'/admins' goals of arranging content on their site. On the other hand, we have theme developers who need a more flexible way of creating regions that different types of content can be dropped into and a web interface that can give admins the ability to choose not only what content gets dropped in, but whether content is dropped into the region at all. And a final issue is whether we have a standard number of regions that have pre-defined relationships that people can count on from theme to theme or whether we leave it entirely up to theme designers a la csszengarden.com. I don't know that there's one simple or easy solution to all of these various needs. The way that this discussion has been going leads me to think that the complexity of this problem isn't fully appreciated despite all of the good suggestions and ideas. Ber Kessel's nice regions mockup [2] only exacerbates the problem because it extends the current head/3-column/footer Drupal paradigm — exactly what we want to move away! I'm not saying that I have a ready solution, but that I think this requires some serious thinking not from the perspective of what's doable given the current block system, but how this whole theme config-theme development ecology should work. I'm extremely concerned about jumping into a partial solution that doesn't fully serve anyone's purposes. I mean, if the best solution is to make theme development easier and more powerful by removing control from the admin, I would support that. If more control for the admin is the best solution at the expense of theme designers flexibility, ok. But I think we need to take a step back and do this right instead of cludge in some ad-hoc solution. To this end, I've been working on an overarching theming proposal... but with the CivicSpace 0.8 release, I haven't had time to devote to it... I'd be interested to hear what people think the IDEAL system for theming would like, rather than something that simply integrates into the existing system... Not because we need to rewrite the whole thing, but because we need fresh ideas and approaches to an obviously complicated problem. [2] http://drupal.org/files/issues/canvas-regions.png -- View: http://drupal.org/node/13008 Edit: http://drupal.org/project/comments/add/13008

1 0

[drupal-devel] [bug] Clean up Administer - Users - Configure
by Steven 27 Jan '05

27 Jan '05

Project: Drupal Version: cvs Component: user.module Category: bug reports Priority: normal Assigned to: Steven Reported by: Steven Updated by: Steven Status: patch Attachment: http://drupal.org/files/issues/clean-up-admin-users.patch (17.73 KB) The current page we have at "administer - users - configure" contains several important pages: roles, permissions, access rules and user profiles. These 4 items are buried too deep and are thus hard to find. I also think that roles/permissions/access rules are not really related to the users (i.e. the people) but more with the accounts that are linked to them. Most things are described by this illustration with Kika-style notes ;) http://acko.net/dumpx/clean-up-admin-users.png This patch also fixes a couple of small things, like adding drupal_gotos or improving the contextual help a bit. Steven -- View: http://drupal.org/node/16271 Edit: http://drupal.org/project/comments/add/16271

5 9

[drupal-devel] unlimited comments displayed on drupal.org is a hack?
by Gabor Hojtsy 27 Jan '05

27 Jan '05

Hi, The stock Drupal only allows setting of comment displays between 10 and 90. Drupal.org clearly has something which makes all comments display. The 'makeover node' for example has 155 comments, all displayed: http://drupal.org/node/11695 Is this some hack in the drupal.org code, or have I seriously missed some setting? It would probably be diserably to provide such a setting on the comment control panel wouldn't it? Goba

3 6

[drupal-devel] [bug] two people editing a node at the same time can result in loss of first changes
by Jeremy 27 Jan '05

27 Jan '05

Project: Drupal Version: cvs -Component: base system +Component: node.module Category: bug reports Priority: normal Assigned to: Jeremy(a)kerneltrap.org Reported by: Jeremy(a)kerneltrap.org Updated by: Jeremy(a)kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/node.module_4.patch (1.29 KB) Resynch. What it does: User A opens node N and starts making changes. User A steps out of the room to grab a quick bite to eat, not yet finished editing node N. User B opens node N and starts making changes. User B saves changes to node N. User A returns from eating, and tries to save changes to node N. With this tiny patch, the node module will detect that node N has changed since user A opened it, and thus will display an error. Prior to this tiny patch, when user A clicked save, all of user B's changes would be lost. How it works: The patch utlizes the existing $node->changed field. Every time the node is previewed or saved, it first checks if $node->changed has been modified in the database. If so, this means that someone has modified the node and an error is displayed. Why it's important: If multiple people are trying to manage a website, it can be very frustrating when one person accidentally over writes another person's changes. Jeremy(a)kerneltrap.org Previous comments: ------------------------------------------------------------------------ February 23, 2004 - 20:58 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/lock.patch (17.88 KB) Drupal currently doesn't provide a generic locking mechanism. As Drupal powered websites get more popular, the lack of locking results in increasingly frequent race conditions. This can result in error messages being cached for an indefinate amount of time. Without locking, it is also possible for content to be edited by two people at the same time. Because of this, it is possible to loose important changes. Attached is a patch that introduces 'lock.inc' and a new 'locks' database table allowing for generic locking functions. It also updates node administration to utilize this new locking functionality and present a powerful example of how this new include file can be used. The attached lock.patch: - installs includes/lock.inc - includes lock.inc from includes/bootstrap.inc - adds lock disable/configure functionality to system.module - wraps administration of nodes in access lock, permitting only one user at a time to edit a node o lock is grabbed on first edit o lock is renewed with each preview o lock is freed with submit or delete o lock will timeout (auto-free) after configurable time period - should work with all node types (tested on story, poll and forum). - only locks administrative editing of nodes. To test, use two web browsers to generate two seperate sessions. More information available here [1]. [1] http://cvs.drupal.org/viewcvs/contributions/sandbox/jeremy/4.4.0/locks/ ------------------------------------------------------------------------ February 23, 2004 - 22:42 : moshe weitzman unless absolutely necessary to serve a cached page, the lock.inc should be included from common.inc, not bootstrap.inc. ------------------------------------------------------------------------ February 23, 2004 - 23:25 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/lock_0.patch (17.88 KB) Updated attachment to include 'lock.inc' from 'common.inc' instead of 'bootstrap.inc'. I can see locking being used to create a cached page, but not when actually serving the cached page... ------------------------------------------------------------------------ February 24, 2004 - 07:38 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/lock_2.patch (17.8 KB) I attached the wrong patch the second time -- it was unchanged from the first patch. This attached patch includes 'lock.inc' from 'common.inc' instead of 'bootstrap.inc'. ------------------------------------------------------------------------ March 1, 2004 - 01:01 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/lock_1.patch (21.99 KB) This updated version of the lock patch is re-synchronized with the latest Drupal HEAD code, and includes the necessary updates to update.inc and the database files. lock.patch modifies the following files: database/database.mssql database/database.mysql database/database.pgsql database/updates.inc includes/common.inc includes/lock.inc modules/node.module ------------------------------------------------------------------------ April 22, 2004 - 00:21 : Anonymous Attachment: http://drupal.org/files/issues/lock_3.patch (21.4 KB) Resynced lock patch against HEAD. Review earlier posts for details Changes: - removed patch to no-longer-existent database.mssql file - moved back to end of updates.inc ------------------------------------------------------------------------ April 23, 2004 - 13:19 : moshe weitzman Dries: it boils down to, how badly do we need it? Sounds like we need to post some use cases here. I'll start: The forum block, the taxonomy_dhtml blocks, and more rely on the drupal cache API to store their data structures. They do so because these structures are expensive to generate, in terms of server resources. When a user posts a new node, these cached structures are deleted. No problem so far. The problem happens when the next several users request a page. They might *simultaneously race* to become the user who generates the blocks and inserts into the cache table. This can spike the server in a bad way. This patch enables forum and taxonomy_dhtml to add a lock such that only 1 user at a time tries to generate the block. As a bonus, this patch comes with nice PHPDoc comments in lock.inc ------------------------------------------------------------------------ April 23, 2004 - 13:44 : Goba Locale cache (in core) and filtercache (contrib module) is also involved. We use these two caches as well on our site, and have seen ugly duplicate cache storage requests by the caching code... ------------------------------------------------------------------------ April 24, 2004 - 08:55 : Jeremy(a)kerneltrap.org Also note that all that lock.inc could be added to core alone, without the rest of this patch. The functionality this patch adds to the node.module is simply one useful example of lock.inc. (However, I am quite happily running with this patch to KernelTrap.org) If merged, my next step is to extend the lock api to also support file-based locks via file.inc. ------------------------------------------------------------------------ June 1, 2004 - 21:07 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/lock_4.patch (15.02 KB) This patch introduces lock.inc, and provides a generic locking mechanism. It wraps cache creation in a lock to prevent errors that have been reported before when two users simultaneous attempt creation of cache content. It also provides the necessary database schema, updating update.inc, etc. Changes to patch: - resynchronized with CVS head. - removed node lock functionality - added lock around cache creation To use: 1) checkout the latest version of Drupal from cvs 2) apply the patch from the top level "patch -p0 < lock.patch" 3) run update.php This patch has been applied to KernelTrap.org (but for 4.4.1) without any problems. ------------------------------------------------------------------------ June 5, 2004 - 14:06 : killes(a)www.drop.org I think this patch should be included. I occassionally see mysql errors due to the fact that Drupal attempts to insert cache for an already present cache ID. ------------------------------------------------------------------------ June 5, 2004 - 14:16 : Goba I often see those SQL errors, using a lot of stuff cached: archive, filtercache, locale, etc. ------------------------------------------------------------------------ June 10, 2004 - 12:30 : killes(a)www.drop.org In the last coupld of days I've had 38 such errors. 30 of them related to the cache and 8 to the history table. I am now testing ths lock.inc patch and will see how many errors I get. ------------------------------------------------------------------------ June 10, 2004 - 19:51 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/node.module-441.patch (966 bytes) Wonderful! Please let me know if you run into any problems, or if you have any suggestions after using lock.inc. BTW: Here's a patch for Drupal 4.4.1 to add lock.inc to node.module, preventing history collisions. It even removes an extra db query that wasn't necessary. ------------------------------------------------------------------------ June 10, 2004 - 19:57 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/node.module-HEAD.patch (966 bytes) And here's the same node.module patch for CVS. ------------------------------------------------------------------------ June 13, 2004 - 03:48 : killes(a)www.drop.org The patch for cvs was not neccessary, the 4.4 version applied just fine. I observe the following: - I get less (only one) cache insert errors. -. I still get some history insert errors (| 7752 | user error: Duplicate entry '281-1116' for key 1) - and: I get lock table insert errors: | 8194 |user error: Duplicate entry '715778' for key 1 query: INSERT INTO locks VALUES('715778', '0.51026200 1086946323', 15, 1, 1) ------------------------------------------------------------------------ June 13, 2004 - 10:32 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/lock_5.patch (1.25 KB) The patch for cvs was not neccessary, the 4.4 version applied just fine. Yes, I didn't notice until I'd uploaded them both that they were identical... ;) - I get less (only one) cache insert errors. Wow, I really didn't think this would be possible. It implies you've hit a race where two lock attempts had the same microtime(). In all my testing, I've been unable to trigger this. In the attached patch I add a sleep that should prevent this. -. I still get some history insert errors (| 7752 | user error: Duplicate entry '281-1116' for key 1) I'm not going to worry about this for now. The history patch is new, and something I know very little about. I may have not put the lock in the correct place. - and: I get lock table insert errors: Do you get a lot if these? Basically, for every time you used to get a cache insert error, do you now get a lock insert error? The use of '@' in the db_query that inserts the lock should prevent this error [2]. I assume you must have something different in your php.ini causing the '@' operator to be ignored, though I've been unable to find any such configurable. The attached patch manually emulates the same functionality, temporarily turning off logging. Can you try the attached patch? It makes small changes to lock.inc. [2] http://us2.php.net/operators.errorcontrol ------------------------------------------------------------------------ July 4, 2004 - 10:38 : Jeremy(a)kerneltrap.org Killes, is there any chance of you testing this with the updated patch? I do not get any race conditions on my installation using lock.inc, and having you try this patch and then provide feedback will help me to understand what's different about your configuration and to create a fully working solution. BTW: What OS/webserver are you using? What version of PHP? ------------------------------------------------------------------------ July 29, 2004 - 04:20 : Bart Jansens Attachment: http://drupal.org/files/issues/20070723.trackback-misc.patch (5.47 KB) The reason the @ operator doesn't work is because drupals error handler simply ignores it. The error handler uses the value of error_reporting only to decide whether or not the error should be printed. There have been patches for this before, but they were never applied for some reason. Assuming this will be fixed someday, do we really need locking in cache_set? Basically the only change is that the errors now occur in lock.inc instead and that a few more queries are executed. Why not just use @db_query in cache_set? oth the lock.inc is useful in many other cases, i've been using it for a few months now to prevent race conditions caused by simultaneous cron runs (poormanscron) and it works great, haven't seen a single race condition since. ------------------------------------------------------------------------ July 29, 2004 - 07:38 : Bart Jansens Ignore that attachment, i didn't even attach it. Must be a project.module bug ------------------------------------------------------------------------ July 29, 2004 - 20:43 : killes(a)www.drop.org Jeremy, sorry for the delay. I have now appied your patch against lock.inc. I actually did only get very few errors in the last weeks (4) and those might be related to some other error not due to file locking at all.. ------------------------------------------------------------------------ July 29, 2004 - 21:59 : Jeremy(a)kerneltrap.org Hi Killes, A few quick questions: 1) are you seeing errors displayed when generating your page, or just in the watchdog logs? 2) did these error go away after applying the patch to lock.inc, or before? 3) what's an example of an error that you're not sure if is lock related? A comment: - lock.inc is in obvious need of a garbage collection function. If you do "select count(*) from locks;" you'll probably get a big number as a result. ------------------------------------------------------------------------ July 29, 2004 - 23:07 : killes(a)www.drop.org 1) I only saw those erros in the watchdog 2) I didn't see any errors after applying this last patch but this is only a few hours ago. 3) Example: user error: Duplicate entry '352-1059' for key 1 query: INSERT INTO history (uid, nid, timestamp) VALUES (352, 1059, 1089978066) in /home/vdst.net/www/htdocs/includes/database.mysql.inc on line 97. I only found three entries in the lock table. The site in question isn't very lively. ------------------------------------------------------------------------ July 29, 2004 - 23:49 : Jeremy(a)kerneltrap.org Killes, I misunderstood your earlier bug report. The lock patch will not fix the collisions showing up in the database. I just submitted another patch [3] that will. The other patch would also benefit the lock patch, as then lock collisions would not have to be reported if the administrator isn't interested in superfluous errors. [3] http://drupal.org/node/view/9620 ------------------------------------------------------------------------ September 1, 2004 - 16:56 : Nick Nassar -1 I think this is the wrong direction to take Drupal. This adds a lot of complexity and doesn't solve the problem. Locks don't work well for interactive multiuser apps because a poorly behaved client can keep a lock out too long. Allowing locks to time out completely erases their usefulness. For example: Alice opens a node for editing (taking out a lock on it) 5 minutes pass, Alice is still writing. The lock times out Bob opens a node for editing Bob saves his changes Alice saves her changes Bob's changes are gone. We should deal with race conditions "lazily," as they happen, rather than explicitly locking resources. We should let the database handle locking. That's what it's good at. MediaWiki (the software used by Wikipedia) does things this way. What should happen is something more along the lines of: db_begin_transaction(); // Check if conditions are okay for this update if ($conditions_ok) { // run queries } else { // Deal with race condition } db_commit_transaction(); Dealing with race conditions for editing a node could be as simple as throwing the user back into the edit screen and complaining that the node has changed since they last edited it. My revisions table patch has implementations of db_begin_transaction() and db_commit_transaction() for both MySQL and PostgreSQL. ------------------------------------------------------------------------ September 15, 2004 - 19:34 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/node.module.patch (1.27 KB) First, it is important to understand what lock.inc accomplishes: it provides a database independent mechanism for temporarily obtaining exclusive rights to a resource. This is the sole objective, and I believe that this works perfectly. Second, you are wrong in your assumption with Bob and Alice. Review the patch and you will see that actually Alice will be unable to save her changes if Bob sneaks in after the lock expired -- instead, she will get an error telling her that someone else has modified the content. Third, your example does not actually prevent racing. It might minimize it, but two seperate people could both check if "conditions are okay" at the same time, and then they could both make their changes. One will win, and one will lose. That is where lock.inc comes in. That said, the attached patch implements something like your "lazy method". It uses the existing 'changed' field for a node to detect if another user has modified the same node you are currently editing, and if so prevents you from saving your changes. In this case, you are returned to the preview page and see the error "This content has been modified by another user, unable to save changes." Please consider this patch for inclusion into 4.5. A small race condition remains which can not be solved without a lock. I will submit another patch to demonstrate the lock if this first patch is accepted. Even without the lock, this is a big improvement to minimize one user accidently walking on another user's changes. ------------------------------------------------------------------------ September 27, 2004 - 13:37 : Anonymous There are two seperate issues here. One is having locks within the program. The other is locking nodes. We obviously need locks within the program to prevent race. As you point out, in my example, we need to make sure that there is no race condition when people check that "conditions are ok." Database transactions/locking allow us to do that in a much simpler way that will still work across different databases, without inventing our own locking system. There is no point in re-inventing the wheel. Locking nodes, on the other hand, is high level user interface behavior. It completely reduces usability. Editing conflicts are comlpicted to deal with, so we should try to minimize the number of times users have to deal with editing conflicts. "Locking" nodes doesn't do this. It makes a node uneditable for a period of time, so if I accidentilly click edit and leave the room, no one else will be able to touch the node until my lock expires. Telling people that they need to re-submit their post because their lock timed out is also very bad behavior. We should be as lazy as possible when dealing with editing conflicts in order to minimize the chance that the end user has to deal with it. ------------------------------------------------------------------------ September 27, 2004 - 14:37 : Nick Nassar That last comment was me. I forgot to log in. Sorry. ------------------------------------------------------------------------ September 27, 2004 - 14:55 : drumm I agree with a few of the others, lets keep this simple. No unnecessary UI or API should be added. That will make everyone's lives easier. For a specific (not entirely thought out) idea maybe "branching" or "conflicting" versions could be built into the revisions system somehow with appropriate messages and UI flow. Can you post a comprehensive and updated patch for a more detailed review? The code seems rather scattered now and I can't figure out what exactly to concentrate on reading to make a better review. Besides node editing, what other use use cases would be possible for lock.inc in core Drupal and contributed modules? ------------------------------------------------------------------------ September 27, 2004 - 15:08 : Holmes Wilson >From a users' perspective, I can't see how it makes sense to lock nodes for an arbitrary time period (here, 5 minutes) after somebody clicks, "edit". If two users both want to contribute to the content on a given node, we want them both to be able to contribute. There's no reason to force users to race the clock in submitting content (5 minutes is not that much time for an intelligent contribution). And it's ENTIRELY UNACCEPTABLE to have users lose the content they submit. If people are spending time making contributions to our site's content, and their changes are lost as soon as they submit them, that's going to demoralize the most helpful participants; if you're running a community-supported site, you can't just ask people for help and then make their contributions disappear into the ether. This patch violates both of those principles. Consider the following situations: Alice takes 15 minutes to finish an edit to a collaboratively editable part of the site. At 6 minutes, when the file is no longer locked, Bob comes along and makes an edit that takes him 5 minutes to finish. The system lets Bob begin his revision, and at 11 minutes Bob posts his revision. Four minutes later Alice unknowingly makes Bob's revision disappear into space. Not cool. We could say "you have 5 minutes to submit your revision, or you might zap someone else's revision" but most people will probably just ignore that (after all, *their* work isn't at stake). We could require them to finish their revisions within 5 minutes, but that won't do much for the quality of our content. And say we extend this 5 minutes to another arbitrary--but longer--15 minutes, to give people time to write. Now say Alice starts a revision and gets an important phonecall, Bob wants to edit the file too, because it contains an embarassing error. Unfortunately for Bob, he's unnecessarily locked out for a good 15 minutes. If your rules are "no potential editor gets turned away" and "nobody's changes get unconsciously destroyed", there's a rather simple solution to this problem: the Wikipedia model. If somebody edits the same page between when you start and finish editing, you get a screen that lets you compare your changes and merge them. Do a little experiment for yourself here: wikipedia.org. The merging screen is not nearly as pretty as it could be, but one could definitely make something better. But the main point is it doesn't violate the two principles "don't reject / demoralize your users" principles. ------------------------------------------------------------------------ September 27, 2004 - 15:34 : tangent Bugzilla [4] supports collision detection, without (I presume) locking. While I haven't looked at the code I am guessing that the time is captured when the form is generated and when the form is submitted a check is made to see if the node was updated between the form start time and the form submit time. I have experienced collisions in bugzilla before and the system simply notifies the user that another user updated the node before you and gives you an opportunity to resubmit. [4] http://www.bugzilla.org/ ------------------------------------------------------------------------ September 27, 2004 - 16:45 : Jeremy(a)kerneltrap.org > Database transactions/locking allow us to do that in a much simpler > way that will still work across different databases, without inventing > our own locking system. There is no point in re-inventing the wheel. I disagree. MySQL 3.x does not support row-level locking nor transactions, and at this time Drupal supports MySQL 3.x. > We should be as lazy as possible when dealing with editing conflicts > in order to minimize the chance that the end user has to deal with it. Indeed. Which is precisely what the patch [5] attached to my previous comment [6] implements. > Can you post a comprehensive and updated patch for a more detailed review? Yes, please refer to the patch [7] attached to my previous comment [8]. (The linked patch does not use any locking -- I have other plans for lock.inc, when time permits.) > I am guessing that the time is captured when the form is generated and when > the form is submitted a check is made to see if the node was updated between the > form start time and the form submit time. This is what I've done in the patch [9] attached to my previous comment [10]. [5] http://drupal.org/files/issues/node.module.patch [6] http://drupal.org/node/6025#comment-12465 [7] http://drupal.org/files/issues/node.module.patch [8] http://drupal.org/node/6025#comment-12465 [9] http://drupal.org/files/issues/node.module.patch [10] http://drupal.org/node/6025#comment-12465 ------------------------------------------------------------------------ September 27, 2004 - 17:40 : Jeremy(a)kerneltrap.org > Besides node editing, what other use use cases >would be possible for lock.inc in core Drupal and >contributed modules? I'm renaming this issue to focus on node editing. I'll start a new issue for locking when I've had a chance to implement recent ideas. I'll address this question at that time. ------------------------------------------------------------------------ October 4, 2004 - 21:19 : Nick Nassar So, why not do table level locking for MySQL 3.x and transactions for databases that support it? It seems to make more sense than writing our own locking system. ------------------------------------------------------------------------ October 5, 2004 - 07:27 : Jeremy(a)kerneltrap.org >So, why not do table level locking for MySQL 3.x and transactions for Please, the patch in question does not have any locking involved at all. It is short, only a few lines, and prevents two people from accidently modifying the same node at the same time. The patch was attached earlier in this thread. You can find it here [11]. [11] http://drupal.org/files/issues/node.module.patch ------------------------------------------------------------------------ October 15, 2004 - 09:47 : moshe weitzman Despite all the chatter in this issue, the final patch is small and useful IMO. ------------------------------------------------------------------------ October 15, 2004 - 09:55 : jhriggs +1 This is a good solution, if only temporary while we may debate more thorough/extensive ways to handle it. ------------------------------------------------------------------------ November 1, 2004 - 18:49 : Nick Nassar I agree. The concept of this latest patch is good, but as Jeremy said, it still has race conditions. Specifically, the following scenario could happen in node_submit(): * Alice starts editing the node * Bob starts editing the node * Alice starts saving the node. * Bob starts saving the node. * Alice's process gets control of the CPU first and passes the line"$node = node_validate($node);" in node_submit() * Bob's process gets the CPU and completely validates and saves the node * Alice's process has already passed the validation line, so it continues saving the node, overwriting Bob's changes. A lock or transaction starting above the $node = node_validate($node); line and ending below node_save() will make sure that the two processes can't enter that region at once and prevent the race condition. We should fix this before we commit it to CVS. If we don't fix the race conditions issue, then sites with heavy traffic will still lose data. ------------------------------------------------------------------------ November 2, 2004 - 07:18 : Jeremy(a)kerneltrap.org The previously attached patch closes the window from minutes, hours, even days, to a second or less. While not 100% perfect (because of the lack of lock), it closes the gap significantly. I say fix this one step at the time, and that this is the logical first step. Putting back to patch, as I'd like to see this merged. Then, if there's interest from the core team members, we can worry about adding a lock. ------------------------------------------------------------------------ November 2, 2004 - 16:00 : Nick Nassar I'm not sure about that. From an end user point-of-view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code. ------------------------------------------------------------------------ November 2, 2004 - 16:01 : Nick Nassar I'm not sure about that. From an end user point-of-view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code. ------------------------------------------------------------------------ November 2, 2004 - 16:30 : Nick Nassar I'm not sure about that. From an end user point-of-view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code. ------------------------------------------------------------------------ November 2, 2004 - 16:31 : Nick Nassar I'm not sure about that. From an end user point of view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. A predictable bug is better than an unpredictable one. I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code. ------------------------------------------------------------------------ November 2, 2004 - 16:32 : Nick Nassar I'm not sure about that. From an end user point of view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. A predictable bug is better than an unpredictable one. I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code. ------------------------------------------------------------------------ November 2, 2004 - 16:41 : Nick Nassar I'm not sure about that. From an end user point of view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. A predictable bug is better than an unpredictable one. I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code. ------------------------------------------------------------------------ November 2, 2004 - 16:42 : Nick Nassar I'm not sure about that. From an end user point of view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. A predictable bug is better than an unpredictable one. I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code. ------------------------------------------------------------------------ November 2, 2004 - 16:44 : Nick Nassar I'm not sure about that. From an end user point of view, making the gap smaller is worse because it will make Drupal seem buggier for people who need to do multiple edits. As is, someone's post it lost every time there's an editing conflict. It's very noticable, and people can intuitively understand what's going on because that's how files work with programs that don't do locking. People who have to edit simultaneously can work around it. If we apply this patch without dealing with the race condition, most of the time it will work, but sometimes you'll lose a post. So, Drupal will seem flakier, even though it will be losing posts due to editing conflicts less often. A predictable bug is better than an unpredictable one. I think it's a good policy, in general, not to commit known buggy code, even if it is arguably less buggy than the current code. ------------------------------------------------------------------------ November 3, 2004 - 20:49 : Jeremy(a)kerneltrap.org Adding locks to Drupal is a larger task, as there are many other places in Drupal that should be wrapped in a lock. Additionally, you will not be able to use the database's own locking functionality because this is not consistent from database to database (or even version to version). I do have a solution for this, but there has been little interest. This patch [12] simply focuses on cleaning up a logic bug. The bug is this: Alice opens a web page to edit it. She takes x minutes to make her changes. During that time, Bob open the same web page to edit it. He takes y minutes to make his changes. Alice finally finishes and saves her changes. Perhaps hours later, Bob finally finishes and saves his changes. Alice's changes have now been lost. This error in logic is fixed by the formentioned patch without introducing locks, simply by utilizing an existing "changed" timestamp. The patch is only a few lines long. You are talking another issue. You are talking about when Alice and Bob both click "save" at the same time. however, if you have not first addressed the problem above, then it becomes much more difficult to address two people clicking save at the same time. I believe we should fix the logic error before we fix the race. Do you have any technical problems with the formentioned patch? Have you looked at it / tried it? [12] http://drupal.org/files/issues/node.module.patch ------------------------------------------------------------------------ November 5, 2004 - 13:28 : Nick Nassar I still say we shouldn't commit known buggy code to CVS, even if it fixes an existing bug. That's not acceptable. -- View: http://drupal.org/node/6025 Edit: http://drupal.org/project/comments/add/6025

1 0

[drupal-devel] [task] Remove admin.module
by Goba 27 Jan '05

27 Jan '05

Project: Drupal Version: cvs Component: admin.module Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: Goba Status: patch +1 on removing the admin module, but why not name the admin page function system_admin_main_page() or something prefixed with 'system', to be consistent? Goba Previous comments: ------------------------------------------------------------------------ January 14, 2005 - 01:55 : drumm Attachment: http://drupal.org/files/issues/system.module_1.patch (1.85 KB) This incredibly small module seems like it should be rolled into system.module to me. Admin.module can be removed once this patch for system.module is applied. -- View: http://drupal.org/node/15570 Edit: http://drupal.org/project/comments/add/15570

7 11