May 2005 - Development - lists.drupal.org

[drupal-devel] [bug] race condition in variable_set in bootstrap.inc
by Dries 06 May '05

06 May '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: Dries Status: patch The proper fix is table/column locking. Dries Previous comments: ------------------------------------------------------------------------ April 7, 2005 - 05: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 - 10: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 - 15: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 - 15: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 - 16: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 - 17: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 9, 2005 - 00: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 9, 2005 - 05: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 - 10: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 - 16: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 ------------------------------------------------------------------------ April 9, 2005 - 17:12 : Jeremy(a)kerneltrap.org 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

1 0

[drupal-devel] [bug] htaccess - rewriteengine giving error 500
by JohnG 06 May '05

06 May '05

Issue status update for http://drupal.org/node/21385 Project: Drupal Version: 4.6.0 Component: file system Category: bug reports Priority: critical Assigned to: Anonymous Reported by: JohnG Updated by: JohnG Status: patch problem: (I'm guessing this falls into the filesystem category ... please feel free to move it if appropriate) immediately following upgrade from 4.5.2 (2005.03.15) to 4.6.0 (on linux-apache-mysql-php server) getting error 500 reports everytime I click a link (from the admin or home page). The problem seems to lie about 3 or 4 lines from the end of the htaccess file: the ReWriteRule is missing a backslash preceding index.php. it did look like this RewriteRule ^(.*)$ index.php?q=$1 [L,QSA] it should look like this RewriteRule ^(.*)$ /index.php?q=$1 [L,QSA] in context, this is the end of the htaccess file, with the correction: # Rewrite current-style URLs of the form 'index.php?q=x'. RewriteCond %{REQUEST_FILENAME} !-f RewriteCond %{REQUEST_FILENAME} !-d RewriteRule ^(.*)$ /index.php?q=$1 [L,QSA] </IfModule> # $Id: .htaccess,v 1.66 2005/03/20 19:15:00 dries Exp $ A number of other solutions have been put forward for this recurring problem. I found this solution from a 4.4 issue at http://drupal.org/node/7836. It seems to work just fine and is obviously preferable to disabling the Htaccess or ReWriteEngine and losing clean URLs. good luck to y'all JohnG JohnG

2 1

[drupal-devel] [bug] menu error line 974 with MBstring
by Dries 06 May '05

06 May '05

Issue status update for http://drupal.org/node/22026 Project: Drupal Version: cvs Component: menu system Category: bug reports Priority: normal Assigned to: Robin Monks Reported by: hendler Updated by: Dries Status: patch I wonder how I can reproduce this problem. Maybe we are fixing the symptom, not the cause. Dries Previous comments: ------------------------------------------------------------------------ May 5, 2005 - 02:15 : hendler Menu.inc on line 974 the do {}while() causes an error if $parent is empty because strrpos, (I have mbstring enabled) may be null to fix my problem I have this if statement wrapping it, which I understand is logically equivlent to just doing a while loop. if ($parent){ do { $parent = substr($parent, 0, strrpos($parent, '/')); } while ($parent && !array_key_exists($parent, $_menu['path index'])); } This happens just after a new install. ------------------------------------------------------------------------ May 6, 2005 - 01:43 : Robin Monks Attachment: http://drupal.org/files/issues/menu.inc_0.patch (908 bytes) We apparently don't check if the object exists before we work on it. Robin

1 0

[drupal-devel] [bug] Strange markup in book render functions
by Dries 06 May '05

06 May '05

Issue status update for http://drupal.org/node/1898 Project: Drupal Version: cvs Component: book.module Category: bug reports Priority: critical Assigned to: puregin Reported by: kika Updated by: Dries Status: patch Let's not abbreviate 'section' to 'sect'. Also, we use '-' to separate words in CSS names. - If the section ID uses the node ID, maybe we should use 'node-' instead of 'sect-'? - If the class uses depth, maybe we should use 'depth-' instead of 'sect'? The same is true for your other patch where you suggest to use 'n'. Better to make that 'node-'. (Feel free to merge both patches.) Looks like the book.module generates a _lot_ of CSS IDs/classes. If the introduction of these new IDs/classes allows us to remove some of the existing IDs/classes, please do. Dries Previous comments: ------------------------------------------------------------------------ June 10, 2003 - 00:06 : kika Snippet from book_render(): $output .= "<dt>". l($node->title, "node/view/$node->nid") ."</dt><dd>". book_body($node) ."<br /><br /></dd>"; Is using definition list here is really correct? This should be replaced with appropiate div's or even better, funnel through theme() Same goes for the function book_print(): $output .= "$node->title"; if ($node->body) { $output .= "<ul>". book_body($node) ."</ul>"; } Why wrap body inside <ul>? ------------------------------------------------------------------------ June 11, 2003 - 02:31 : al Yeah, it's all nasty. I patched this ages ago, but no one seemed interested. The patch now won't apply due to all the changes since then. I'm on this - expect a new patch to hit my sandbox shortly. ------------------------------------------------------------------------ June 11, 2003 - 08:03 : ax please remove the superfluous <ul>s and <li> [1], too. to solve the problem of invalid nested <hx> tags, maybe we should change <h$depth> to <h1 class="book-h$depth">? this way, we could use <h2> - <h6> in book pages. alternatively, we should only use <h5> and <h6> in book pages - we shouldn't have nestings deeper than 4 in printed books, then. additionally, i think it would be a good idea to add id's to all book headings so they can be jumped to via fragment identifiers (url#fragment-identifier). i often wish this would be possible when referencing the last paragraph of a long page of documentation ... these id's should be of the form <hx id="foo" name="foo"> [2], with foo probably being the node-id of the single book page. adding id's to headings would also be a good idea for headings / sections in single book nodes. for example, it would be nice if i could link to list point 10 in the drupal installation instructions [3] via something like url#10. these id's had to be added manually. the question is: how can these id's be named to be unique within a /printed/ book? probably <node_id>_<section_id> ... [1] [ [drupal-devel] Drupal handbook formatting and heading tags | http://lists.drupal.org/pipermail/drupal-devel/2003-June/025860.html ] [2] [ XHTML Spec, HTML Compatibility Guidelines, Fragment Identifiers | http://www.w3.org/TR/xhtml1/#C_8 ] [3] http://drupal.org/node/view/260 ------------------------------------------------------------------------ June 15, 2003 - 16:23 : al Fixed in latest CVS. Uses all of Ax's suggestions: - <h1 class="depthX"> - Fixed to use CSS rather than tables. - Changes link at top to standard "breadcrumb" style, rather than hierarchy in tree view. - Removes odd and superfluous <ul> tags and the like. - Makes printed docs have proper <html> and <body> wrapping, etc. ------------------------------------------------------------------------ May 5, 2005 - 09:20 : puregin I'm re-opening this, because the problem persists in CVS. 1) The current CVS version (1.288.2.4) of book module still wraps $node->body in an <ul> element in book_print_recurse(). This produces non-conformant HTML. I'm not sure why this was done in the first place - I'm guessing someone wanted to insulate from having bad markup in a book page throw off the enclosing page. Getting rid of the offending UL tags is a one-line solution, but what is the right thing to do here? I'd like to see something very structural - wrapping the /entire/ output (including the generated sectional header) of the book_print_recurse() wrapped in a <div> element. This will properly nest true structural sections, making the structure accessible to applications via DOM. It will also make it easier to use XSLT, for example, to transform into various XML based export formats, for example. Any reason not to do so? Also... when we get the 'printer-friendly' version of a page, the subtree rooted at that page is returned, and the root page is marked up with a class="book-h1". Is there any reason why we don't compute the depth of the root page, and use that to generate the "in-place" header? In other words, if a section is "book-h3" relative to the top-level book, shouldn't it be "book-h3" whenever it is viewed? In this case, we could apply the same principal to generating the <div> tags around each section. ------------------------------------------------------------------------ May 6, 2005 - 00:58 : puregin Attachment: http://drupal.org/files/issues/book_printer_friendly_div.patch (1.57 KB) I am attaching a patch which fixes improper printer-friendly output, as discussed. The patch fixes book_print and book_print_recurse() to insert a <div> start tag before emitting the header, and to close this after subsections have been recursively generated. The <div> tag has an id attribute of the form id="sect-123", where the node's nid is 123, and a class attribute of form class="sect/n/" where the depth of the section is /n/. I'm marking this issue as critical, since the present HTML output plays havoc with any attempts at sane PDF generation. I will address the issue of where printer-friendly subsections should be 'rooted' in a follow-up. Djun

1 0

[drupal-devel] [bug] Fix search box in phptemplate core themes
by kbahey 06 May '05

06 May '05

Issue status update for http://drupal.org/node/22127 Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: normal Assigned to: kbahey Reported by: kbahey Updated by: kbahey Status: patch Attachment: http://drupal.org/files/issues/phptemplate-core-themes.patch (1.86 KB) Both core phptemplate themes use name="keys" instead of name="edit[keys]", which was introduced since 4.6. This would cause the search box not to work. The attached patch fixes this. kbahey

1 0

[drupal-devel] [bug] menu error line 974 with MBstring
by Robin Monks 06 May '05

06 May '05

Issue status update for http://drupal.org/node/22026 Project: Drupal -Version: 4.6.0 +Version: cvs Component: menu system Category: bug reports Priority: normal -Assigned to: Anonymous +Assigned to: Robin Monks Reported by: hendler Updated by: Robin Monks -Status: active +Status: patch Attachment: http://drupal.org/files/issues/menu.inc_0.patch (908 bytes) We apparently don't check if the object exists before we work on it. Robin Robin Monks Previous comments: ------------------------------------------------------------------------ May 5, 2005 - 00:15 : hendler Menu.inc on line 974 the do {}while() causes an error if $parent is empty because strrpos, (I have mbstring enabled) may be null to fix my problem I have this if statement wrapping it, which I understand is logically equivlent to just doing a while loop. if ($parent){ do { $parent = substr($parent, 0, strrpos($parent, '/')); } while ($parent && !array_key_exists($parent, $_menu['path index'])); } This happens just after a new install.

1 0

[drupal-devel] [bug] Section depth for printer-friendly output off-by-one error
by puregin 06 May '05

06 May '05

Issue status update for http://drupal.org/node/22123 Project: Drupal Version: cvs Component: book.module Category: bug reports Priority: critical Assigned to: puregin Reported by: puregin Updated by: puregin Status: patch Attachment: http://drupal.org/files/issues/book_printer_friendly_depth.patch (583 bytes) Proper subsections of a top-level section in printer-friendly output have level equal to the parent. That is, we have Structure: Expected depth Actual depth section 1 1 1 section 1.1 2 1 section 1.1.1 3 2 section 1.2 2 1 The attached patch is a one-line change. It increments $depth before calling book_print_recurse() from book_print(). I'm marking this as critical since it is interfering with generation of PDF from the Handbook. Djun puregin

1 0

[drupal-devel] [bug] Strange markup in book render functions
by puregin 06 May '05

06 May '05

Issue status update for http://drupal.org/node/1898 Project: Drupal Version: cvs Component: book.module Category: bug reports -Priority: normal +Priority: critical -Assigned to: al +Assigned to: puregin Reported by: kika Updated by: puregin -Status: active +Status: patch Attachment: http://drupal.org/files/issues/book_printer_friendly_div.patch (1.57 KB) I am attaching a patch which fixes improper printer-friendly output, as discussed. The patch fixes book_print and book_print_recurse() to insert a <div> start tag before emitting the header, and to close this after subsections have been recursively generated. The <div> tag has an id attribute of the form id="sect-123", where the node's nid is 123, and a class attribute of form class="sect/n/" where the depth of the section is /n/. I'm marking this issue as critical, since the present HTML output plays havoc with any attempts at sane PDF generation. I will address the issue of where printer-friendly subsections should be 'rooted' in a follow-up. Djun puregin Previous comments: ------------------------------------------------------------------------ June 9, 2003 - 14:06 : kika Snippet from book_render(): $output .= "<dt>". l($node->title, "node/view/$node->nid") ."</dt><dd>". book_body($node) ."<br /><br /></dd>"; Is using definition list here is really correct? This should be replaced with appropiate div's or even better, funnel through theme() Same goes for the function book_print(): $output .= "$node->title"; if ($node->body) { $output .= "<ul>". book_body($node) ."</ul>"; } Why wrap body inside <ul>? ------------------------------------------------------------------------ June 10, 2003 - 16:31 : al Yeah, it's all nasty. I patched this ages ago, but no one seemed interested. The patch now won't apply due to all the changes since then. I'm on this - expect a new patch to hit my sandbox shortly. ------------------------------------------------------------------------ June 10, 2003 - 22:03 : ax please remove the superfluous <ul>s and <li> [1], too. to solve the problem of invalid nested <hx> tags, maybe we should change <h$depth> to <h1 class="book-h$depth">? this way, we could use <h2> - <h6> in book pages. alternatively, we should only use <h5> and <h6> in book pages - we shouldn't have nestings deeper than 4 in printed books, then. additionally, i think it would be a good idea to add id's to all book headings so they can be jumped to via fragment identifiers (url#fragment-identifier). i often wish this would be possible when referencing the last paragraph of a long page of documentation ... these id's should be of the form <hx id="foo" name="foo"> [2], with foo probably being the node-id of the single book page. adding id's to headings would also be a good idea for headings / sections in single book nodes. for example, it would be nice if i could link to list point 10 in the drupal installation instructions [3] via something like url#10. these id's had to be added manually. the question is: how can these id's be named to be unique within a /printed/ book? probably <node_id>_<section_id> ... [1] [ [drupal-devel] Drupal handbook formatting and heading tags | http://lists.drupal.org/pipermail/drupal-devel/2003-June/025860.html ] [2] [ XHTML Spec, HTML Compatibility Guidelines, Fragment Identifiers | http://www.w3.org/TR/xhtml1/#C_8 ] [3] http://drupal.org/node/view/260 ------------------------------------------------------------------------ June 15, 2003 - 06:23 : al Fixed in latest CVS. Uses all of Ax's suggestions: - <h1 class="depthX"> - Fixed to use CSS rather than tables. - Changes link at top to standard "breadcrumb" style, rather than hierarchy in tree view. - Removes odd and superfluous <ul> tags and the like. - Makes printed docs have proper <html> and <body> wrapping, etc. ------------------------------------------------------------------------ May 4, 2005 - 23:20 : puregin I'm re-opening this, because the problem persists in CVS. 1) The current CVS version (1.288.2.4) of book module still wraps $node->body in an <ul> element in book_print_recurse(). This produces non-conformant HTML. I'm not sure why this was done in the first place - I'm guessing someone wanted to insulate from having bad markup in a book page throw off the enclosing page. Getting rid of the offending UL tags is a one-line solution, but what is the right thing to do here? I'd like to see something very structural - wrapping the /entire/ output (including the generated sectional header) of the book_print_recurse() wrapped in a <div> element. This will properly nest true structural sections, making the structure accessible to applications via DOM. It will also make it easier to use XSLT, for example, to transform into various XML based export formats, for example. Any reason not to do so? Also... when we get the 'printer-friendly' version of a page, the subtree rooted at that page is returned, and the root page is marked up with a class="book-h1". Is there any reason why we don't compute the depth of the root page, and use that to generate the "in-place" header? In other words, if a section is "book-h3" relative to the top-level book, shouldn't it be "book-h3" whenever it is viewed? In this case, we could apply the same principal to generating the <div> tags around each section.

1 0

[drupal-devel] [bug] 404 when file_transfer(.,.) fails
by Robin Monks 06 May '05

06 May '05

Issue status update for http://drupal.org/node/22118 Project: Drupal Version: cvs Component: file system Category: bug reports Priority: normal Assigned to: Robin Monks Reported by: Robin Monks Updated by: Robin Monks Status: patch Attachment: http://drupal.org/files/issues/404 on file not found.patch (699 bytes) "The image module uses file_transfer and if for any reason the file is gone, then you'll get an infinte number of error messages which -- because they are in a -- are invisible..." Chx_ told me about this bug, so volia! patch attached. This patch checks to ensure that the file can be accessed and generenates a drupal_not_found if it hits a snag. Robin Robin Monks

1 1

[drupal-devel] [bug] Do not execute functions in include files
by chx 05 May '05

05 May '05

Issue status update for http://drupal.org/node/18213 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: chx Reported by: killes(a)www.drop.org Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/boot_3.patch (17.98 KB) The database layer is now totally independent of other parts of Drupal. chx Previous comments: ------------------------------------------------------------------------ March 1, 2005 - 23:09 : killes(a)www.drop.org Drupal currently executes functions in include files. This is very annoying if you want to include files from external scripts such as update scripts or other scripts that want to use Drupal as a framework. Offending files are: database.inc: db_set_active(); session.inc: session_set_save_handler("sess_open", "sess_close", "sess_read", "sess_write", "sess_destroy", "sess_gc"); session_start(); common.inc: A bunch of stuff at the bottom. All this should be moved into a start.inc file. ------------------------------------------------------------------------ March 27, 2005 - 07:44 : adrian I haven't done this recently, but back in 4.4 era I attempted something similar to this for the install api. A _lot_ of modifications were required to be able to use (for instance) the theme api without needing to have a database connection active. ------------------------------------------------------------------------ March 27, 2005 - 07:55 : adrian It's definately not a simple problem. For instance.. the db connection needs to be started during bootstrap, and during normal execution. I think the better idea is to include a DRUPAL_INIT constant we can check before executing the function. (or a DRUPAL_NO_INIT .. i'm not picky). ------------------------------------------------------------------------ April 28, 2005 - 22:53 : killes(a)www.drop.org Here's an old patch by me that illustrates what I want to do. http://lists.drupal.org/archives/drupal-devel/2005-03/txtq23pAcHATw.txt Instead of putting the stuff into index.php, alternative file names need to be found. ------------------------------------------------------------------------ May 4, 2005 - 18:36 : chx Attachment: http://drupal.org/files/issues/boot.patch (6.83 KB) Well, I have written something to address the issue. Now you can include everything to your hearts content just include init.inc as well and do an appropriate drupal_bootstrap. Alas, ATM database and bootstrap is very interwined. This interdependency could be obliterated if we'd not use variable_get('dev_query') in database.*.inc. cvs diff -u -N -F^f against cvs.drupal.org as CVSROOT failed to include to include the two new files, so I'll include them as followups. ------------------------------------------------------------------------ May 4, 2005 - 18:37 : chx Attachment: http://drupal.org/files/issues/boot_0.patch (10.31 KB) Hm, settings.php changes were ommited. ------------------------------------------------------------------------ May 4, 2005 - 18:37 : chx Attachment: http://drupal.org/files/issues/init.inc (3.46 KB) ------------------------------------------------------------------------ May 4, 2005 - 18:38 : chx Attachment: http://drupal.org/files/issues/settings_ini_set.php (850 bytes) ------------------------------------------------------------------------ May 4, 2005 - 18:44 : chx Attachment: http://drupal.org/files/issues/boot_1.patch (14.49 KB) Following advice from darix I have now created one patch. Still, I'd like to know how could I do this with cvs. ------------------------------------------------------------------------ May 4, 2005 - 19:01 : chx Attachment: http://drupal.org/files/issues/boot_2.patch (14.48 KB) Steef found that I had $$ in settings.php. He also asked what's this good for. Several things, but most advantages come when you use only parts of Drupal. If you want use, say, only our forms functions then now you can include('common.inc'); without side effects. If you need the database layer, you do a include('init.inc') and call drupal_bootstrap('database')

1 0