April 2005 - Development - lists.drupal.org

[drupal-devel] [bug] Private downloads aren't working
by chx 11 Apr '05

11 Apr '05

Issue status update for http://drupal.org/node/12267 Project: Drupal Version: cvs Component: file system Category: bug reports Priority: critical Assigned to: Anonymous Reported by: stefan nagtegaal Updated by: chx Status: patch This is the link for the file: <?php $file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())) ?> and file_create_url <?php case FILE_DOWNLOADS_PRIVATE: return url('system/files', 'file='. $path); ?> basically, the preview link and the final link is very, very differnt in private case: if it's a preview (no fid) you get a URL without a query unlike in the final stage. To say the least, it's not trivial to fix. As Dries said: private file downloads need to be rewritten. chx Previous comments: ------------------------------------------------------------------------ October 29, 2004 - 10:37 : stefan nagtegaal The private downloads are not working properly.. How to reproduce: Goto 'http://www.your-site.com/admin/settings' and set the 'Download method' to 'Private - files are transferred by Drupal'. Now, you make a new item (blog/story/page/poll, whatever) and upload an image. You should use this image inside your post, by doing an '' inside your post, where $path is the url which is given in the 'Attachments' form-group on the submission page. (Something like 'system/files?file=$yourfile.ext') Then click 'Preview' and do not submit the node! Now, have a look what we see.. We see that the image inside the post isn't displayed properly. The image is broken. If we ask the properties of the image it points to the right location ('system/files?file=$yourfile.ext'), but isn't displayed. If we take a look at the properties of the table under the node (attached files table with 'Attachement', and 'List') we see that the attachment is pointing to something like 'http://www.frontaal-online.com/$file-system-path/$yourfile.ext' instead of the private download url.. I'm not sure how to fix this, but I think it's a bug inside function file_create_url() which could be found inside file.inc. ------------------------------------------------------------------------ October 29, 2004 - 10:51 : stefan nagtegaal Attachment: http://drupal.org/files/issues/upload-and-body.png (19.87 KB) Screenshot after upload en filling in the body.form ------------------------------------------------------------------------ October 29, 2004 - 10:52 : stefan nagtegaal Attachment: http://drupal.org/files/issues/node-preview-and-image-properties.png (78.18 KB) Now we are previewing the node with a broken image in it.. See the properties of the image, which points to another location as the 'metallic.png' hyperlink inside the 'Attachment'-table under the node preview. ------------------------------------------------------------------------ October 29, 2004 - 10:54 : stefan nagtegaal Attachment: http://drupal.org/files/issues/properties-of-metallic-link-in-attachments-t… (53.52 KB) The properties of the 'metallic.png'-link inside the table under the node preview differs from where the source points us to.. The url inside the table works, (which is the same as in public_dowloads), but the url inside the body isn't... ------------------------------------------------------------------------ October 29, 2004 - 11:47 : stefan nagtegaal This bug is also appearing inside drupal 4.5... ------------------------------------------------------------------------ January 28, 2005 - 17:04 : walkah Attachment: http://drupal.org/files/issues/upload-private-files.patch (1.88 KB) here's a small patch to fix. basically the custom url rewriting (for when files haven't been yet saved to their final location) wasn't taking into account FILE_DOWNLOADS_PRIVATE. this fixes that (oh, and it also fixes an sql error in upload_download as well). ------------------------------------------------------------------------ January 28, 2005 - 17:07 : walkah Attachment: http://drupal.org/files/issues/upload-4-5.patch (1.36 KB) here's a version for 4.5 as well ... (where the SQL wasn't broken). ------------------------------------------------------------------------ February 5, 2005 - 08:34 : Steven Isn't that SQL 'error' there to make query rewriting work? ------------------------------------------------------------------------ February 5, 2005 - 08:36 : Steven By the way, I still think we should alter the autop filter so it uses & rather than making this one use a numbered entity. 'amp' is the most common notation out there, and it's shorter too. ------------------------------------------------------------------------ March 13, 2005 - 19:14 : killes(a)www.drop.org Doesn't apply anymore. Is this still an issue? ------------------------------------------------------------------------ April 7, 2005 - 02:56 : moshe weitzman this one is marked critical. Walkah, Stefan - please comment on whether this is still a bug. ------------------------------------------------------------------------ April 8, 2005 - 01:13 : Shane Birley This might point some people in the correct direction. It isn't completely the same issue, but related, I would think and might need to be fixed prior. http://drupal.org/node/17364 ------------------------------------------------------------------------ April 8, 2005 - 09:38 : stefan nagtegaal This is still an issue in 4.6 RC and can still be reproduced by follwing exactly what I propose above.. While this is a longstanding bug, and with drupal 4.6 I'll changed status to patch.. Maybe some other people ould have a look at this?

1 0

[drupal-devel] [task] Add missing help text for book.module
by JonBob 10 Apr '05

10 Apr '05

Issue status update for http://drupal.org/node/19884 Project: Drupal Version: cvs Component: forum.module Category: tasks Priority: normal Assigned to: Anonymous Reported by: JonBob Updated by: JonBob Status: active Help texts are needed for these pages: - "?q=admin/forum/configure" http://lists.drupal.org/archives/drupal-docs/2005-04/msg00008.html JonBob

3 2

[drupal-devel] [feature] Always show node->type on node edit page
by Robin Monks 10 Apr '05

10 Apr '05

Issue status update for http://drupal.org/node/20228 Project: Drupal Version: cvs Component: node system Category: feature requests Priority: normal Assigned to: Robin Monks Reported by: robertDouglass Updated by: Robin Monks Status: patch Attachment: http://drupal.org/files/issues/Edit Title_0.patch (701 bytes) Actualy, I made an error in that patch. This should be correct. Robin Robin Monks Previous comments: ------------------------------------------------------------------------ April 9, 2005 - 18:27 : robertDouglass Wen creating a node there is a nice h1 title saying "Submit {node type}". When returning to the node edit form later this information is gone. Many node types are indistinguishable from another based on the form alone (page, story, blog...), so I think it would be a rather nice feature to always state which node type is being edited. ------------------------------------------------------------------------ April 9, 2005 - 22:01 : Robin Monks Taking request... ------------------------------------------------------------------------ April 9, 2005 - 22:57 : Robin Monks Attachment: http://drupal.org/files/issues/Edit Title.patch (601 bytes) This patch shows "Editing /contenttype/ /contenttitle/". Example: "Editing blog As the World Turns". Robin Monks moznetwork.org

1 0

[drupal-devel] [feature] Always show node->type on node edit page
by Robin Monks 10 Apr '05

10 Apr '05

Issue status update for http://drupal.org/node/20228 Project: Drupal Version: cvs Component: node system Category: feature requests Priority: normal Assigned to: Robin Monks Reported by: robertDouglass Updated by: Robin Monks -Status: active +Status: patch Attachment: http://drupal.org/files/issues/Edit Title.patch (601 bytes) This patch shows "Editing /contenttype/ /contenttitle/". Example: "Editing blog As the World Turns". Robin Monks moznetwork.org Robin Monks Previous comments: ------------------------------------------------------------------------ April 9, 2005 - 18:27 : robertDouglass Wen creating a node there is a nice h1 title saying "Submit {node type}". When returning to the node edit form later this information is gone. Many node types are indistinguishable from another based on the form alone (page, story, blog...), so I think it would be a rather nice feature to always state which node type is being edited. ------------------------------------------------------------------------ April 9, 2005 - 22:01 : Robin Monks Taking request...

1 0

[drupal-devel] [bug] Blocks with no title kill Drupal
by Robin Monks 09 Apr '05

09 Apr '05

Issue status update for http://drupal.org/node/19965 Project: Drupal Version: cvs Component: block.module Category: bug reports Priority: normal -Assigned to: chx +Assigned to: Robin Monks Reported by: budda Updated by: Robin Monks Status: patch Attachment: http://drupal.org/files/issues/drupal_0.patch (3.67 KB) This new patch no longer requires a unique value for title, and also checks to ensure the Block Description is not a duplicate or empty value. Big thanks go out to chx for assistance. Robin Robin Monks Previous comments: ------------------------------------------------------------------------ April 4, 2005 - 17:03 : budda When setting more than one block's title to '' (blank) Drupal dies, and doesn't catch the SQL query error nicely. The error is as follows: user error: Duplicate entry '' for key 2 query: UPDATE boxes SET title = '', body = ' ...<cut>... in /var/www/html/drupal-cvs/includes/database.mysql.inc on line 66. Using 4.6 build from 4/4/2005 The page ends with: warning: Cannot modify header information - headers already sent by (output started at /var/www/html/drupal-cvs/includes/common.inc:384) in /var/www/html/drupal-cvs/includes/common.inc on line 192. ------------------------------------------------------------------------ April 8, 2005 - 08:08 : robertDouglass There should be a check before the insert is attempted and if the block title already exists the user should be notified with form_error(). ------------------------------------------------------------------------ April 8, 2005 - 09:46 : Poolio this certainly doesn't solve the problem, but I've worked around this issue by using html tags in the title field for blocks that I don't want to show a title. ------------------------------------------------------------------------ April 9, 2005 - 00:31 : Robin Monks Attachment: http://drupal.org/files/issues/block.module.blank.title.patch (955 bytes) A small patch that checks to ensure the title does not already exit in the Blocks list. Also, if the field was left blank, in adds an HTML comment with the current date to the second to ensure that a duplicate title isn't created. This is my first patch, so I probably did something wrong, but I'm willing to fix it too. Robin Monks MozNetwork.org ------------------------------------------------------------------------ April 9, 2005 - 01:22 : Morbus Iff I'm, personally, fine with how you've fixed the blank block title. You've really solved two problems in one: duplicate titles (which is the cause of this Issue), and the desire to have blank (or no) titles (which is the reason for this Issue) for the end user. With that said, I'm not a fan of your error message (but, then again, anal wording is my specialty, so don't think I'm picking on you): I'd rename it to "That block title is already in use; please choose another." The goals here: The user may not remember all the previous titles he's used. Using "unique" starts forcing him to remember. We shouldn't do that. The user already knows he entered it - we don't have to repeat that to him. It's active, not passive (no use of "have"). Use a semi-colon, not a comma. It's much shorter and concise. ------------------------------------------------------------------------ April 9, 2005 - 06:27 : chx Attachment: http://drupal.org/files/issues/box_title.patch (1.49 KB) Why display any message at all? ------------------------------------------------------------------------ April 9, 2005 - 14:11 : Morbus Iff Displaying no message at all would only work if the user wasn't typing in ANY title whatsoever (since then we'd make one for them based on the date). But, if a user names a block "Users" one week, and then creates another block called "Users" three weeks later, than an whoopsidaisy error needs to be shown - we shouldn't modify the text randomly. ------------------------------------------------------------------------ April 9, 2005 - 15:10 : chx Well, let's discuss the main point: why shoiuld be the title unique? ------------------------------------------------------------------------ April 9, 2005 - 15:38 : Morbus Iff Are you asking why Drupal enforces them being unique, or why, visually and to the end-user, they should be unique? ------------------------------------------------------------------------ April 9, 2005 - 15:43 : chx I guess Drupal forces them 'cos for some reasons it is thought that for the users they should be unique. This is not the case, however. Yes, it's confusing if they are on the same page. But I can think on circumstances where they would not be on the same page, so the force from Drupal seems unnecessary. ------------------------------------------------------------------------ April 9, 2005 - 19:00 : robertDouglass agree with chx on the unique point. Like the functionality of adding html comments and timestamp (why timestamp? why not a block id?) if left blank if they must be uniqe. Would be in favor of making the visible title completely optional. Administrators have description to keep them separate on the admin screen.

1 0

[drupal-devel] [bug] Blocks with no title kill Drupal
by robertDouglass 09 Apr '05

09 Apr '05

Issue status update for http://drupal.org/node/19965 Project: Drupal Version: cvs Component: block.module Category: bug reports Priority: normal Assigned to: chx Reported by: budda Updated by: robertDouglass Status: patch agree with chx on the unique point. Like the functionality of adding html comments and timestamp (why timestamp? why not a block id?) if left blank if they must be uniqe. Would be in favor of making the visible title completely optional. Administrators have description to keep them separate on the admin screen. robertDouglass Previous comments: ------------------------------------------------------------------------ April 4, 2005 - 18:03 : budda When setting more than one block's title to '' (blank) Drupal dies, and doesn't catch the SQL query error nicely. The error is as follows: user error: Duplicate entry '' for key 2 query: UPDATE boxes SET title = '', body = ' ...<cut>... in /var/www/html/drupal-cvs/includes/database.mysql.inc on line 66. Using 4.6 build from 4/4/2005 The page ends with: warning: Cannot modify header information - headers already sent by (output started at /var/www/html/drupal-cvs/includes/common.inc:384) in /var/www/html/drupal-cvs/includes/common.inc on line 192. ------------------------------------------------------------------------ April 8, 2005 - 09:08 : robertDouglass There should be a check before the insert is attempted and if the block title already exists the user should be notified with form_error(). ------------------------------------------------------------------------ April 8, 2005 - 10:46 : Poolio this certainly doesn't solve the problem, but I've worked around this issue by using html tags in the title field for blocks that I don't want to show a title. ------------------------------------------------------------------------ April 9, 2005 - 01:31 : Robin Monks Attachment: http://drupal.org/files/issues/block.module.blank.title.patch (955 bytes) A small patch that checks to ensure the title does not already exit in the Blocks list. Also, if the field was left blank, in adds an HTML comment with the current date to the second to ensure that a duplicate title isn't created. This is my first patch, so I probably did something wrong, but I'm willing to fix it too. Robin Monks MozNetwork.org ------------------------------------------------------------------------ April 9, 2005 - 02:22 : Morbus Iff I'm, personally, fine with how you've fixed the blank block title. You've really solved two problems in one: duplicate titles (which is the cause of this Issue), and the desire to have blank (or no) titles (which is the reason for this Issue) for the end user. With that said, I'm not a fan of your error message (but, then again, anal wording is my specialty, so don't think I'm picking on you): I'd rename it to "That block title is already in use; please choose another." The goals here: The user may not remember all the previous titles he's used. Using "unique" starts forcing him to remember. We shouldn't do that. The user already knows he entered it - we don't have to repeat that to him. It's active, not passive (no use of "have"). Use a semi-colon, not a comma. It's much shorter and concise. ------------------------------------------------------------------------ April 9, 2005 - 07:27 : chx Attachment: http://drupal.org/files/issues/box_title.patch (1.49 KB) Why display any message at all? ------------------------------------------------------------------------ April 9, 2005 - 15:11 : Morbus Iff Displaying no message at all would only work if the user wasn't typing in ANY title whatsoever (since then we'd make one for them based on the date). But, if a user names a block "Users" one week, and then creates another block called "Users" three weeks later, than an whoopsidaisy error needs to be shown - we shouldn't modify the text randomly. ------------------------------------------------------------------------ April 9, 2005 - 16:10 : chx Well, let's discuss the main point: why shoiuld be the title unique? ------------------------------------------------------------------------ April 9, 2005 - 16:38 : Morbus Iff Are you asking why Drupal enforces them being unique, or why, visually and to the end-user, they should be unique? ------------------------------------------------------------------------ April 9, 2005 - 16:43 : chx I guess Drupal forces them 'cos for some reasons it is thought that for the users they should be unique. This is not the case, however. Yes, it's confusing if they are on the same page. But I can think on circumstances where they would not be on the same page, so the force from Drupal seems unnecessary.

1 0

[drupal-devel] [bug] Blocks with no title kill Drupal
by chx 09 Apr '05

09 Apr '05

Issue status update for http://drupal.org/node/19965 Project: Drupal Version: cvs Component: block.module Category: bug reports Priority: normal Assigned to: chx Reported by: budda Updated by: chx Status: patch I guess Drupal forces them 'cos for some reasons it is thought that for the users they should be unique. This is not the case, however. Yes, it's confusing if they are on the same page. But I can think on circumstances where they would not be on the same page, so the force from Drupal seems unnecessary. chx Previous comments: ------------------------------------------------------------------------ April 4, 2005 - 18:03 : budda When setting more than one block's title to '' (blank) Drupal dies, and doesn't catch the SQL query error nicely. The error is as follows: user error: Duplicate entry '' for key 2 query: UPDATE boxes SET title = '', body = ' ...<cut>... in /var/www/html/drupal-cvs/includes/database.mysql.inc on line 66. Using 4.6 build from 4/4/2005 The page ends with: warning: Cannot modify header information - headers already sent by (output started at /var/www/html/drupal-cvs/includes/common.inc:384) in /var/www/html/drupal-cvs/includes/common.inc on line 192. ------------------------------------------------------------------------ April 8, 2005 - 09:08 : robertDouglass There should be a check before the insert is attempted and if the block title already exists the user should be notified with form_error(). ------------------------------------------------------------------------ April 8, 2005 - 10:46 : Poolio this certainly doesn't solve the problem, but I've worked around this issue by using html tags in the title field for blocks that I don't want to show a title. ------------------------------------------------------------------------ April 9, 2005 - 01:31 : Robin Monks Attachment: http://drupal.org/files/issues/block.module.blank.title.patch (955 bytes) A small patch that checks to ensure the title does not already exit in the Blocks list. Also, if the field was left blank, in adds an HTML comment with the current date to the second to ensure that a duplicate title isn't created. This is my first patch, so I probably did something wrong, but I'm willing to fix it too. Robin Monks MozNetwork.org ------------------------------------------------------------------------ April 9, 2005 - 02:22 : Morbus Iff I'm, personally, fine with how you've fixed the blank block title. You've really solved two problems in one: duplicate titles (which is the cause of this Issue), and the desire to have blank (or no) titles (which is the reason for this Issue) for the end user. With that said, I'm not a fan of your error message (but, then again, anal wording is my specialty, so don't think I'm picking on you): I'd rename it to "That block title is already in use; please choose another." The goals here: The user may not remember all the previous titles he's used. Using "unique" starts forcing him to remember. We shouldn't do that. The user already knows he entered it - we don't have to repeat that to him. It's active, not passive (no use of "have"). Use a semi-colon, not a comma. It's much shorter and concise. ------------------------------------------------------------------------ April 9, 2005 - 07:27 : chx Attachment: http://drupal.org/files/issues/box_title.patch (1.49 KB) Why display any message at all? ------------------------------------------------------------------------ April 9, 2005 - 15:11 : Morbus Iff Displaying no message at all would only work if the user wasn't typing in ANY title whatsoever (since then we'd make one for them based on the date). But, if a user names a block "Users" one week, and then creates another block called "Users" three weeks later, than an whoopsidaisy error needs to be shown - we shouldn't modify the text randomly. ------------------------------------------------------------------------ April 9, 2005 - 16:10 : chx Well, let's discuss the main point: why shoiuld be the title unique? ------------------------------------------------------------------------ April 9, 2005 - 16:38 : Morbus Iff Are you asking why Drupal enforces them being unique, or why, visually and to the end-user, they should be unique?

1 0

[drupal-devel] [bug] Blocks with no title kill Drupal
by Morbus Iff 09 Apr '05

09 Apr '05

Issue status update for http://drupal.org/node/19965 Project: Drupal Version: cvs Component: block.module Category: bug reports Priority: normal Assigned to: chx Reported by: budda Updated by: Morbus Iff Status: patch Are you asking why Drupal enforces them being unique, or why, visually and to the end-user, they should be unique? Morbus Iff Previous comments: ------------------------------------------------------------------------ April 4, 2005 - 12:03 : budda When setting more than one block's title to '' (blank) Drupal dies, and doesn't catch the SQL query error nicely. The error is as follows: user error: Duplicate entry '' for key 2 query: UPDATE boxes SET title = '', body = ' ...<cut>... in /var/www/html/drupal-cvs/includes/database.mysql.inc on line 66. Using 4.6 build from 4/4/2005 The page ends with: warning: Cannot modify header information - headers already sent by (output started at /var/www/html/drupal-cvs/includes/common.inc:384) in /var/www/html/drupal-cvs/includes/common.inc on line 192. ------------------------------------------------------------------------ April 8, 2005 - 03:08 : robertDouglass There should be a check before the insert is attempted and if the block title already exists the user should be notified with form_error(). ------------------------------------------------------------------------ April 8, 2005 - 04:46 : Poolio this certainly doesn't solve the problem, but I've worked around this issue by using html tags in the title field for blocks that I don't want to show a title. ------------------------------------------------------------------------ April 8, 2005 - 19:31 : Robin Monks Attachment: http://drupal.org/files/issues/block.module.blank.title.patch (955 bytes) A small patch that checks to ensure the title does not already exit in the Blocks list. Also, if the field was left blank, in adds an HTML comment with the current date to the second to ensure that a duplicate title isn't created. This is my first patch, so I probably did something wrong, but I'm willing to fix it too. Robin Monks MozNetwork.org ------------------------------------------------------------------------ April 8, 2005 - 20:22 : Morbus Iff I'm, personally, fine with how you've fixed the blank block title. You've really solved two problems in one: duplicate titles (which is the cause of this Issue), and the desire to have blank (or no) titles (which is the reason for this Issue) for the end user. With that said, I'm not a fan of your error message (but, then again, anal wording is my specialty, so don't think I'm picking on you): I'd rename it to "That block title is already in use; please choose another." The goals here: The user may not remember all the previous titles he's used. Using "unique" starts forcing him to remember. We shouldn't do that. The user already knows he entered it - we don't have to repeat that to him. It's active, not passive (no use of "have"). Use a semi-colon, not a comma. It's much shorter and concise. ------------------------------------------------------------------------ April 9, 2005 - 01:27 : chx Attachment: http://drupal.org/files/issues/box_title.patch (1.49 KB) Why display any message at all? ------------------------------------------------------------------------ April 9, 2005 - 09:11 : Morbus Iff Displaying no message at all would only work if the user wasn't typing in ANY title whatsoever (since then we'd make one for them based on the date). But, if a user names a block "Users" one week, and then creates another block called "Users" three weeks later, than an whoopsidaisy error needs to be shown - we shouldn't modify the text randomly. ------------------------------------------------------------------------ April 9, 2005 - 10:10 : chx Well, let's discuss the main point: why shoiuld be the title unique?

1 0

[drupal-devel] [bug] race condition in variable_set in bootstrap.inc
by Jeremy 09 Apr '05

09 Apr '05

Issue status update for http://drupal.org/node/20114 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: critical Assigned to: Ricetons Reported by: Ricetons Updated by: Jeremy(a)kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/variable_set_1.patch (1.04 KB) Just because it's possible, here's an alternative patch that never costs more than 1 db_query. Costs: - if the variable is not previously set: one db query - if the variable is previously set, and changes: one db query - if the variable is previously set, and not changing: no db queries Jeremy(a)kerneltrap.org Previous comments: ------------------------------------------------------------------------ April 6, 2005 - 22:53 : Ricetons This bug was exposed by the poormancron module. We can see that the module itself contains no bug. But in the function variable_get(), the function just execute SQL like this: DELETE bar WHERE key="foo" INSERT INTO bar VALUES(foo, bar2ooo) but suppose this: thread A and thread B has executed the DELETE sentence. And they do INSERT at the same time. then an SQL error like this occured: duplicated key 'foo' in table "variables" It's a critical bug. It should be fixed before the time that drupal 4.6.0 released. ------------------------------------------------------------------------ April 8, 2005 - 03:01 : robertDouglass I also experience this on a site that has many different cron cycles ranging from 2 minutes to 6 hours. The query errors which arise (as noted above) are impossible to reproduce through normal testing yet occur fairly (surprisingly) often in the logs. This doesn't just apply to cron jobs but rather to all code that calls variable_set. Setting to CVS since this hasn't been addressed since the issue was opened. ------------------------------------------------------------------------ April 8, 2005 - 08:10 : jhriggs Attachment: http://drupal.org/files/issues/variable_set_update.patch (922 bytes) I'm not sure that there is a "right" way to fix this. In mysql, we could use a REPLACE rather than DELETE/INSERT, but that wouldn't work for pgsql. And, if we are really seeing this type of race, there is no telling which REPLACE would actually make it last and be saved. Instead, we could do a check to see if the row exists and do an UPDATE if it does and an INSERT otherwise; however, this would be prone to the same problem if we are seeing such a race. That is, they may both see that the row does not exist and both attempt to do an INSERT. This will only happen the first time a variable is created, though, which may be more acceptable than what we are seeing now. If the row does already exist, they would both do UPDATES and would "succeed" in the sense that there is no duplicate key error. There is still no telling which one actually sticks, though. The attached patch takes the second approach. ------------------------------------------------------------------------ April 8, 2005 - 08:37 : Jeremy(a)kerneltrap.org What keeps another session from doing a DELETE after your session does its SELECT COUNT but before it does its UPDATE? Or, what keeps another session from doing an INSERT after your session does its SELECT COUNT but before it does its INSERT? All you've done is add another query... I think you'll need to use locking to fix this right. ------------------------------------------------------------------------ April 8, 2005 - 09:46 : jhriggs 1. Assuming that all we are talking about is variable_set() calls, DELETE is not an issue, because the patch removes deletes. It only does updates and inserts. Now if you throw in variable_del() calls, that's another story, but it's also outside the scope of this issue. It would likely be much more rare than the delete/insert problems we see now. 2. Your second example is the only lingering downfall. As I mentioned in my post: "...they may both see that the row does not exist and both attempt to do an INSERT. This will only happen the first time a variable is created, though, which may be more acceptable than what we are seeing now." 3. This doesn't add any queries. Right now it does DELETE and INSERT. With the patch it does SELECT and UPDATE/INSERT. Two queries either way. As I alluded, this may not be a true fix -- I don't think there is one for race conditions like this, even with table-locking -- but it addresses the problem in a way that makes sense and eliminates an error in the majority of cases. ------------------------------------------------------------------------ April 8, 2005 - 10:42 : killes(a)www.drop.org @Jeremy: db locking will become available with the revisios patch. I don't advise to use it too mch because it is very slow... I actually do not see why this needs a fix. All it is going to produce is a message in the error log... ------------------------------------------------------------------------ April 8, 2005 - 17:06 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/variable_set.patch (708 bytes) The attached patch makes more sense to me. The same method is used by cache_set() and statistics_exit(). Once a variable already exists in the variable table, we only need to do one db_query to update it... ------------------------------------------------------------------------ April 8, 2005 - 22:39 : Ricetons variable_set() is a function being used almost everywhere in drupal. INSERT/DELETE caused a serious performance problem. Maybe we should come to a soultion which uses a miminum number of queries. In my opinion, it seems that the third patch can solve the problem, but it's not good enough. By the way, table locking costs too much. I don't agree to use that method to solve this case. ------------------------------------------------------------------------ April 9, 2005 - 03:37 : Steven Jeremy: cache_set(), and thus this patch, suffer from an annoying bug: db_affected_rows() only returns which rows were changed, not which rows matched the WHERE clause. This means that if you set a variable to exactly the same value as it already is, that a db error will be generated. This is rarely the case with cache and only occurs in a race condition there, but with variables this problem is quite important. ------------------------------------------------------------------------ April 9, 2005 - 09:07 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/variable_set_0.patch (978 bytes) Okay, here's a fix for variable_set. It has the added advantage of not requiring any db_queries if we're trying to set the variable to a value it's already equal to. (Granted, without using locks there's always a tiny race window... But this will always be true.) Costs: - if the variable is not previously set: two db queries - if the variable is previously set, and changes: one db query - if the variable is previously set, and not changing: no db query

1 0

[drupal-devel] [bug] Blocks with no title kill Drupal
by chx 09 Apr '05

09 Apr '05

Issue status update for http://drupal.org/node/19965 Project: Drupal Version: cvs Component: block.module Category: bug reports Priority: normal Assigned to: chx Reported by: budda Updated by: chx Status: patch Well, let's discuss the main point: why shoiuld be the title unique? chx Previous comments: ------------------------------------------------------------------------ April 4, 2005 - 18:03 : budda When setting more than one block's title to '' (blank) Drupal dies, and doesn't catch the SQL query error nicely. The error is as follows: user error: Duplicate entry '' for key 2 query: UPDATE boxes SET title = '', body = ' ...<cut>... in /var/www/html/drupal-cvs/includes/database.mysql.inc on line 66. Using 4.6 build from 4/4/2005 The page ends with: warning: Cannot modify header information - headers already sent by (output started at /var/www/html/drupal-cvs/includes/common.inc:384) in /var/www/html/drupal-cvs/includes/common.inc on line 192. ------------------------------------------------------------------------ April 8, 2005 - 09:08 : robertDouglass There should be a check before the insert is attempted and if the block title already exists the user should be notified with form_error(). ------------------------------------------------------------------------ April 8, 2005 - 10:46 : Poolio this certainly doesn't solve the problem, but I've worked around this issue by using html tags in the title field for blocks that I don't want to show a title. ------------------------------------------------------------------------ April 9, 2005 - 01:31 : Robin Monks Attachment: http://drupal.org/files/issues/block.module.blank.title.patch (955 bytes) A small patch that checks to ensure the title does not already exit in the Blocks list. Also, if the field was left blank, in adds an HTML comment with the current date to the second to ensure that a duplicate title isn't created. This is my first patch, so I probably did something wrong, but I'm willing to fix it too. Robin Monks MozNetwork.org ------------------------------------------------------------------------ April 9, 2005 - 02:22 : Morbus Iff I'm, personally, fine with how you've fixed the blank block title. You've really solved two problems in one: duplicate titles (which is the cause of this Issue), and the desire to have blank (or no) titles (which is the reason for this Issue) for the end user. With that said, I'm not a fan of your error message (but, then again, anal wording is my specialty, so don't think I'm picking on you): I'd rename it to "That block title is already in use; please choose another." The goals here: The user may not remember all the previous titles he's used. Using "unique" starts forcing him to remember. We shouldn't do that. The user already knows he entered it - we don't have to repeat that to him. It's active, not passive (no use of "have"). Use a semi-colon, not a comma. It's much shorter and concise. ------------------------------------------------------------------------ April 9, 2005 - 07:27 : chx Attachment: http://drupal.org/files/issues/box_title.patch (1.49 KB) Why display any message at all? ------------------------------------------------------------------------ April 9, 2005 - 15:11 : Morbus Iff Displaying no message at all would only work if the user wasn't typing in ANY title whatsoever (since then we'd make one for them based on the date). But, if a user names a block "Users" one week, and then creates another block called "Users" three weeks later, than an whoopsidaisy error needs to be shown - we shouldn't modify the text randomly.

1 0