January 2005 - Development - lists.drupal.org

[drupal-devel] [bug] PHP Error when accessing settings page
by Anonymous 17 Feb '05

17 Feb '05

Project: Drupal Version: cvs Component: system.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Anonymous Updated by: Anonymous Status: patch When accessing the admin...settings page on a subdomain site, I get the following two errors: warning: fsockopen(): php_network_getaddresses: getaddrinfo failed: Name or service not known in /home/username/public_html/includes/common.inc on line 227. warning: fsockopen(): unable to connect to sub.domain.com:80 in /home/username/public_html/includes/common.inc on line 227. This is a result of a failure to resolve the subdomain at my host. (DNS is hosted elsewhere from the server.) This could be easily fixed by adding an "@" before $request = drupal_http_request($GLOBALS['base_url'] . '/system/test'); in system.module. I don't have an environment set up to create a patch at the moment, but the change is very minor. Anonymous -- View: http://drupal.org/node/16309 Edit: http://drupal.org/project/comments/add/16309

1 1

[drupal-devel] [task] usability: redirect to proper page after edit/delete operations
by moshe weitzman 12 Feb '05

12 Feb '05

Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: moshe weitzman Reported by: moshe weitzman Updated by: moshe weitzman Status: patch Attachment: http://drupal.org/files/issues/drdest.patch (11.05 KB) Here is a patch I've been wanting to finish for a while. This patch assures that you end up on the proper page after you edit/delete a node, comment, user, or url alias. This is true no matter if you go through the usual interface or the admin interface. Further, if click the 'edit' link from 3rd page of a custom sorted view (e.g. admin/comment&from=100&sort=asc&order=Author) you still are returned to the right page. The technique used here is generally available for module developers. I've minimally enhanced drupal_goto() so that it will redirect to the url specified in a 'destination' querystring parameter if such parameter exists. If it does not exist, we redirect just as today. No changes are required to existing drupal_goto() calls. A new helper function, drupal_get_destination() was added; it helps contruct the 'destination' string which is appended to add/edit links. The only downside I can see to this patch is that a few URLs are less pretty than before. These urls are only shown to admins. This could only be avoided by having each admin page implement its own way of passing a destination, or stashing the destination in the $_SESSION. We recently tried storing referer in $_SESSION, and it was eventually removed because of poor coordination when a user has multiple browser windows open. In addition to the above, - I cleaned up some 'destination' handling in user login code - I assured that after adding a new taxo term, we arrive back on the 'Add' page. That restores prior behavior moshe weitzman -- View: http://drupal.org/node/16246 Edit: http://drupal.org/project/comments/add/16246

9 10

[drupal-devel] Should we make project issue status more nuanced?
by Nedjo Rogers 10 Feb '05

10 Feb '05

Following up on recent discussions on drupal decision-making and structure (thanks for the many comments, particularly Steven's recent succinct summary, http://drupal.org/node/15916#comment-26377) I'd like to look at some practical improvements to the system we use for evaluating patches, see my reply to Steven's comment at http://drupal.org/node/15916#comment-26627. I know I could suggest this as a patch on the project module, but I'd like to start with a more general testing of the waters. And, if your eyes are glazing over as you mutter "not him again with his structure babble", please bear with me, I think we might be on to something. The basic issue is that too much is put on the single status, "patch". To see the problem in action, we only have to do a search on Drupal project issues with status=patch. I currently get a total of 124 matches, some ranging up to 33 weeks old. I'm not suggesting the CVS review team is doing anything but excellent work. I know that for my part I can't properly keep up even with the issues for the modules I maintain. I have nothing but appreciation for the work Dries, Steven, and Kjartan do. But they're not able on their own to work through everything. When I go through the outstanding patches, I find quite a few that look (to my not wholly qualified eye) ready to apply--that is, they received discussion, support was evinced, issues raised were to all appearances addressed. Some proposals were very small, but useful. Often the only obvious missing factor is that the patch is now outdated, having sat for so long. Others don't meet these criteria (e.g., lack demonstrated support). What happens currently is that an issue jumps straight from "patch" to "applied"--that is, the status change comes *after* the decision-maker (most often, Dries) has made the change. What's missing is a way for Drupal contributors - more specifically, the subset of developers who are qualified to do so - to pre-filter the patches--basically, to put them in a queue for approval, indicating that, in their view, the patch meets our approval criteria (set out in the Handbook at http://drupal.org/node/10262) Concretely, I'm thinking of an additional status category after "patch": * "reviewed" or "tentatively approved" This would be set when a qualified Drupal member (e.g., a maintainer, or other recognized core contributor) judged the patch meets our criteria. To help even further to increase the community role in decision-making (and thereby relieve the pressure or bottleneck on the CVS review team), we could also consider a time factor. I'm thinking that, when a given amount of time has passed after a patch was deemed "tentatively approved" and no further issues have been raised, the patch would (likely, automatically) move to a second additional category: * "pending application" providing a further indication of readiness. Of course, the actual decision would remain with the CVS review team, who could still raise issues (downgrade the status, deny the proposal, etc.). As an additional step, we could create a new drupal.org role, and limit the ability to set these new status categories to users in this role (i.e., recognized core contributors/developers). Thus we could make sure this judgement was made by qualified members (and would be roughly equivalent to what I think we do e.g. for documentation--a role with permissions to edit the Handbook). What to people think of these suggestions? Would they help improve our code review system? Thanks, Nedjo Rogers

7 9

[drupal-devel] [feature] generalizing node_rewrite_sql
by chx 05 Feb '05

05 Feb '05

Project: Drupal Version: cvs Component: database system Category: feature requests Priority: normal Assigned to: chx Reported by: chx Updated by: chx Status: patch Attachment: http://drupal.org/files/issues/db_rewrite_sql.patch (7.66 KB) As I was thinking on the various usages of node_rewrite_sql , it came to my mind that with very little changes it can be generalized so that any queries against any table -- not just node -- can be rewritten. I have moved node_rewrite_sql to database.inc, renamed it db_rewrite_sql, introduced another necessary parameter, which does not affect the current (light) usage of node_rewrite_sql. I think the usage is even clearer now, 'cos I think nid_alias caused some confusion. Now it is called $primary_table , while the new parameter is called $primary_key. What this would be good for? One may want to have permission and languages for taxonomy terms and vocabularies. chx -- View: http://drupal.org/node/16111 Edit: http://drupal.org/project/comments/add/16111

7 19

[drupal-devel] [task] Core image handling
by walkah 01 Feb '05

01 Feb '05

Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: walkah Reported by: walkah Updated by: walkah Status: patch Attachment: http://drupal.org/files/issues/image_0.inc (7.44 KB) Due to popular demand - attached here is an image.inc for inclusion (in includes/), that will offer some basic image detection and manipulation functionality to drupal's core. currently image.inc is responsible for managing image 'toolkits' - these are either PHP modules (e.g. GD) or external programs (e.g. imagemagick) that handle the actual manipulation of images. also included is the default "GD" toolkit (that supports both GD1 or GD2, depending on which is installed). PHP comes bundled with gd as of version 4.3 - therefore this is a reasonable default. However, other toolkits will follow (I presume) - they can be installed in includes/ (as image.$toolkit.inc). There are 5 main functions that modules may now utilize to handle images: * image_get_info() - this function checks a file - if it exists and is a valid image file , it will return an array containing things like the pixel dimensions of the image, plus the 'type' and common extension. * image_scale - resizes a given image to fit within a given width / height dimensions, while maintaining aspect ratio (not distorting the image). - this function can be used to generate thumbnails, or ensure a maximum resolution, etc. * image_resize - similar to image_scale (but will not respect aspect ratio - may well distort the image). * image_rotate - rotate an image by X degrees * image_crop - crops an image to a given rectangle (defined as top-left x/y coordinates plus a width & height of the rectangle). In a follow up I will post a patch that does the following: * adds some default settings to admin/settings : * toolkit selection (and possibly configuration) * default thumbnail dimensions (WxH) - to be used as a system-wide default * maximum dimensions (WxH ) - to be used as a system-wide default * changes the user picture handling to silently resize uploaded pictures to fit within the specified dimensions (rather than throwing errors). * uses image_get_info() to verify logo uploads in admin/theme configuration * patches upload.module to respect maximum dimensions for uploaded images. Contribution modules will now be able to rely on these base manipulation functions to offer additional functionality (such as image nodes, photo galleries, advanced image manipulation, etc) walkah -- View: http://drupal.org/node/16358 Edit: http://drupal.org/project/comments/add/16358

13 23

[drupal-devel] [feature] More flexible rss output
by walkah 01 Feb '05

01 Feb '05

Project: Drupal Version: cvs Component: node system Category: feature requests Priority: normal Assigned to: walkah Reported by: walkah Updated by: walkah Status: patch Attachment: http://drupal.org/files/issues/format_rss_item.patch (4.06 KB) this patch does a few things: * alters format_rss_item (in common.inc) to allow the $args paramater to be an array of arrays (optionally), so that extra arguments can have attributes - i.e. think enclosures * introduces a nodeapi option for 'rss item', so that other modules can add additional info to each rss item * patches taxonomy.module to add category information to feeds * patches upload module to add enclosure for feeds (NOTE: rss only allows 1 enclosure per item). this would allow things like, say, podcasts to be available in drupal. walkah -- View: http://drupal.org/node/16513 Edit: http://drupal.org/project/comments/add/16513

5 6

[drupal-devel] [task] Rewrite to use procedures and conditionally include xmlrpc.inc
by jvandyk 01 Feb '05

01 Feb '05

Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: walkah Reported by: killes(a)www.drop.org Updated by: jvandyk Status: patch And now that I've revisited JonBob's comments on this, I can see why it's currently the way it is and can't be conditionally included. Blech. jvandyk Previous comments: ------------------------------------------------------------------------ April 29, 2004 - 18:15 : killes(a)www.drop.org Drupal includes xmlrpc.inc for every non-cached page view. However, the functions from the file are only rarely used. Therefore we need a patch that includes the file from the functions that want to use it. ------------------------------------------------------------------------ May 2, 2004 - 18:30 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc.patch (4.64 KB) Here is a patch that moves the include statement to the functiosn where xmlrpc.inc is actually needed. The patch coul dneed some testing as I do not use those modules (ping, bloggerapi) myself. ------------------------------------------------------------------------ May 3, 2004 - 21:34 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_0.patch (6.38 KB) Patch updated, I had forgotten drupal.module. thanks to crschmidt for spotting this. ------------------------------------------------------------------------ May 3, 2004 - 22:17 : Steven drupal_xml_parser_create() does not require xmlrpc.inc... ------------------------------------------------------------------------ May 3, 2004 - 22:27 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_1.patch (6.06 KB) Ok, updated. ------------------------------------------------------------------------ May 3, 2004 - 22:41 : Dries If xmlrpc.php includes xmlrpc.inc, why do we have to include it in blogapi module's functions once more? Also, the inclusion of xmlrpc.inc in drupal.module can probably be avoided by including xmlrpc.inc in cron.php? Am I missing something? ------------------------------------------------------------------------ May 3, 2004 - 22:49 : crschmidt Not all of the tasks in drupal.module are run with cron.php: for example, the distributed authentication comes in through drupal.module, which requires code from xmlrpc.inc to create the messages it sends out, as far as I can tell. So, the drupal.module needs it for distributed authentication stuff, not just for pinging servers and so on. ------------------------------------------------------------------------ May 3, 2004 - 23:38 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_2.patch (2.8 KB) "If xmlrpc.php includes xmlrpc.inc, why do we have to include it in blogapi module's functions once more?" As I said, I am not that familiar with that part of Drupal, did not investigate under whch circumstances blogapi functions could be called, and wanted to stay on the safe side. If you are sure that blogapi is always called from xmlrpcs.php, then apply the appended version. ------------------------------------------------------------------------ May 4, 2004 - 05:40 : Dries I'm not 100% sure either, I was merely suggesting an alternative. I think the second patch might work tough, in which case it would be preferred for its simplicity. Not? I'll add "Testing Gerhard's blog API patch to my short term TODO list". Hopefully I get to it tonight after work. Thanks Gerhard. ------------------------------------------------------------------------ May 12, 2004 - 20:12 : Kjartan Good patch, just a few minor comments. - Use single quotes for constant strings. - I noticed some indentations that were off the 2 column, looks like the are in the current code though. - drupal_directory_ping() will only be invoked from xmlrpc.php so there shouldn't be a need to include the file there. ------------------------------------------------------------------------ May 12, 2004 - 22:40 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_2_0.patch (2.64 KB) Updated. Don't know to which spaces you are referring. Guess Drupal CVS needs to be code checked again. ------------------------------------------------------------------------ August 4, 2004 - 01:39 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_3.patch (2.81 KB) Updated again. It appears that parts of the old patch made it into drupal.module, but the include statement was still in common.inc. ------------------------------------------------------------------------ August 4, 2004 - 05:47 : TDobes +1... I tested the patch with blogapi.module and w.bloggar and it works fine, as far as I can tell. I like any patch that includes less code on each page view. ------------------------------------------------------------------------ August 4, 2004 - 20:36 : Dries Committed to HEAD. Thanks. ------------------------------------------------------------------------ August 13, 2004 - 18:47 : walkah Attachment: http://drupal.org/files/issues/xmlrpc-include.diff (2.38 KB) this patch didn't break blogapi.module, it's true. but it _did_ break drupal.module's distributed authentication feature. the problem is that xmlrpc.inc declares some classes, which will not work if the include statement appears inside a function. see the following for more details: http://us4.php.net/manual/en/language.functions.php (specifically the comment by albaity at php4web dot com) the attached patch restores the drupal.module functionality (as well as reintroduces the include in common.inc). I'm afraid that's just the way it has to be, sorry killes ;) (this patch also fixes a typo in drupal.module where the 'name' array key was not quoted - which may break things under higher error reporting levels). ------------------------------------------------------------------------ August 13, 2004 - 19:23 : Anonymous Can't you include common.inc from drupal.module if it needs that file? locale.module also includes its locale.inc without punishing the user who doesn't care about its functionality. Gerhard ------------------------------------------------------------------------ August 14, 2004 - 06:29 : Dries Including the XML-RPC library at the top of those files/modules that need it sounds like a fair alternative. ------------------------------------------------------------------------ August 14, 2004 - 11:19 : Steven But that doesn't really help unless you have no XML-RPC modules enabled at all... ------------------------------------------------------------------------ August 14, 2004 - 12:58 : JonBob What is more, I don't think it even fixes the bug. The problem is that class declarations in PHP4 are not guaranteed to work correctly when made inside a function. This applies to declarations immediately inside a function definition, or in files included from inside a function definition, or even from files included from files included from function definitions. Note that since module files are included from module_load(), this means class definitions or includes of files that have them cannot safely be in modules. Basically, conditional inclusion of classes is a no-no. To get around this we'd have to rewrite xmlrpc.inc using functions rather than object methods. ------------------------------------------------------------------------ August 16, 2004 - 18:29 : Dries I reverted the patch. ------------------------------------------------------------------------ August 17, 2004 - 15:39 : walkah ya, but you forgot to put the include_once back in common.inc . :) ------------------------------------------------------------------------ August 17, 2004 - 17:59 : killes(a)www.drop.org Can't just somebody step forward and rewrite that class in functions? ------------------------------------------------------------------------ August 31, 2004 - 21:41 : killes(a)www.drop.org I'd like to keep this open. It is so nicely assigned. ;-> ------------------------------------------------------------------------ September 1, 2004 - 12:35 : Anonymous *grumble* ------------------------------------------------------------------------ January 31, 2005 - 23:05 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmplrpc.patch (695 bytes) Here is an alternative approach. XML-RPC requests are suppoed to be POST requests, not GET. By jvandyk and myself. ------------------------------------------------------------------------ January 31, 2005 - 23:22 : jvandyk This patch affects the inclusion of xmlrpc, which is the xmlrpc client, not xmlrpcs, which is the server and subject to the "all XML-RPC requests are POST requests" rule. Thus, it is ineffective. However, we should consider eliminating the inclusion of xmlrpc.inc at all in common.inc. This would speed up Drupal at the expense of modules that use XML-RPC calls having to include xmlrpc.inc themselves. grepping for xmlrpc_ finds occurrences in drupal.module for cross-drupal-site authentication, foaf.module, livejournalmodule, and waypath.module. -- View: http://drupal.org/node/7458 Edit: http://drupal.org/project/comments/add/7458

1 0

[drupal-devel] [task] Rewrite to use procedures and conditionally include xmlrpc.inc
by jvandyk 01 Feb '05

01 Feb '05

Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: walkah Reported by: killes(a)www.drop.org Updated by: jvandyk Status: patch This patch affects the inclusion of xmlrpc, which is the xmlrpc client, not xmlrpcs, which is the server and subject to the "all XML-RPC requests are POST requests" rule. Thus, it is ineffective. However, we should consider eliminating the inclusion of xmlrpc.inc at all in common.inc. This would speed up Drupal at the expense of modules that use XML-RPC calls having to include xmlrpc.inc themselves. grepping for xmlrpc_ finds occurrences in drupal.module for cross-drupal-site authentication, foaf.module, livejournalmodule, and waypath.module. jvandyk Previous comments: ------------------------------------------------------------------------ April 29, 2004 - 18:15 : killes(a)www.drop.org Drupal includes xmlrpc.inc for every non-cached page view. However, the functions from the file are only rarely used. Therefore we need a patch that includes the file from the functions that want to use it. ------------------------------------------------------------------------ May 2, 2004 - 18:30 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc.patch (4.64 KB) Here is a patch that moves the include statement to the functiosn where xmlrpc.inc is actually needed. The patch coul dneed some testing as I do not use those modules (ping, bloggerapi) myself. ------------------------------------------------------------------------ May 3, 2004 - 21:34 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_0.patch (6.38 KB) Patch updated, I had forgotten drupal.module. thanks to crschmidt for spotting this. ------------------------------------------------------------------------ May 3, 2004 - 22:17 : Steven drupal_xml_parser_create() does not require xmlrpc.inc... ------------------------------------------------------------------------ May 3, 2004 - 22:27 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_1.patch (6.06 KB) Ok, updated. ------------------------------------------------------------------------ May 3, 2004 - 22:41 : Dries If xmlrpc.php includes xmlrpc.inc, why do we have to include it in blogapi module's functions once more? Also, the inclusion of xmlrpc.inc in drupal.module can probably be avoided by including xmlrpc.inc in cron.php? Am I missing something? ------------------------------------------------------------------------ May 3, 2004 - 22:49 : crschmidt Not all of the tasks in drupal.module are run with cron.php: for example, the distributed authentication comes in through drupal.module, which requires code from xmlrpc.inc to create the messages it sends out, as far as I can tell. So, the drupal.module needs it for distributed authentication stuff, not just for pinging servers and so on. ------------------------------------------------------------------------ May 3, 2004 - 23:38 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_2.patch (2.8 KB) "If xmlrpc.php includes xmlrpc.inc, why do we have to include it in blogapi module's functions once more?" As I said, I am not that familiar with that part of Drupal, did not investigate under whch circumstances blogapi functions could be called, and wanted to stay on the safe side. If you are sure that blogapi is always called from xmlrpcs.php, then apply the appended version. ------------------------------------------------------------------------ May 4, 2004 - 05:40 : Dries I'm not 100% sure either, I was merely suggesting an alternative. I think the second patch might work tough, in which case it would be preferred for its simplicity. Not? I'll add "Testing Gerhard's blog API patch to my short term TODO list". Hopefully I get to it tonight after work. Thanks Gerhard. ------------------------------------------------------------------------ May 12, 2004 - 20:12 : Kjartan Good patch, just a few minor comments. - Use single quotes for constant strings. - I noticed some indentations that were off the 2 column, looks like the are in the current code though. - drupal_directory_ping() will only be invoked from xmlrpc.php so there shouldn't be a need to include the file there. ------------------------------------------------------------------------ May 12, 2004 - 22:40 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_2_0.patch (2.64 KB) Updated. Don't know to which spaces you are referring. Guess Drupal CVS needs to be code checked again. ------------------------------------------------------------------------ August 4, 2004 - 01:39 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_3.patch (2.81 KB) Updated again. It appears that parts of the old patch made it into drupal.module, but the include statement was still in common.inc. ------------------------------------------------------------------------ August 4, 2004 - 05:47 : TDobes +1... I tested the patch with blogapi.module and w.bloggar and it works fine, as far as I can tell. I like any patch that includes less code on each page view. ------------------------------------------------------------------------ August 4, 2004 - 20:36 : Dries Committed to HEAD. Thanks. ------------------------------------------------------------------------ August 13, 2004 - 18:47 : walkah Attachment: http://drupal.org/files/issues/xmlrpc-include.diff (2.38 KB) this patch didn't break blogapi.module, it's true. but it _did_ break drupal.module's distributed authentication feature. the problem is that xmlrpc.inc declares some classes, which will not work if the include statement appears inside a function. see the following for more details: http://us4.php.net/manual/en/language.functions.php (specifically the comment by albaity at php4web dot com) the attached patch restores the drupal.module functionality (as well as reintroduces the include in common.inc). I'm afraid that's just the way it has to be, sorry killes ;) (this patch also fixes a typo in drupal.module where the 'name' array key was not quoted - which may break things under higher error reporting levels). ------------------------------------------------------------------------ August 13, 2004 - 19:23 : Anonymous Can't you include common.inc from drupal.module if it needs that file? locale.module also includes its locale.inc without punishing the user who doesn't care about its functionality. Gerhard ------------------------------------------------------------------------ August 14, 2004 - 06:29 : Dries Including the XML-RPC library at the top of those files/modules that need it sounds like a fair alternative. ------------------------------------------------------------------------ August 14, 2004 - 11:19 : Steven But that doesn't really help unless you have no XML-RPC modules enabled at all... ------------------------------------------------------------------------ August 14, 2004 - 12:58 : JonBob What is more, I don't think it even fixes the bug. The problem is that class declarations in PHP4 are not guaranteed to work correctly when made inside a function. This applies to declarations immediately inside a function definition, or in files included from inside a function definition, or even from files included from files included from function definitions. Note that since module files are included from module_load(), this means class definitions or includes of files that have them cannot safely be in modules. Basically, conditional inclusion of classes is a no-no. To get around this we'd have to rewrite xmlrpc.inc using functions rather than object methods. ------------------------------------------------------------------------ August 16, 2004 - 18:29 : Dries I reverted the patch. ------------------------------------------------------------------------ August 17, 2004 - 15:39 : walkah ya, but you forgot to put the include_once back in common.inc . :) ------------------------------------------------------------------------ August 17, 2004 - 17:59 : killes(a)www.drop.org Can't just somebody step forward and rewrite that class in functions? ------------------------------------------------------------------------ August 31, 2004 - 21:41 : killes(a)www.drop.org I'd like to keep this open. It is so nicely assigned. ;-> ------------------------------------------------------------------------ September 1, 2004 - 12:35 : Anonymous *grumble* ------------------------------------------------------------------------ January 31, 2005 - 23:05 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmplrpc.patch (695 bytes) Here is an alternative approach. XML-RPC requests are suppoed to be POST requests, not GET. By jvandyk and myself. -- View: http://drupal.org/node/7458 Edit: http://drupal.org/project/comments/add/7458

1 0

[drupal-devel] [task] Rewrite to use procedures and conditionally include xmlrpc.inc
by killes 01 Feb '05

01 Feb '05

Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: walkah Reported by: killes(a)www.drop.org Updated by: killes(a)www.drop.org -Status: active +Status: patch Attachment: http://drupal.org/files/issues/xmplrpc.patch (695 bytes) Here is an alternative approach. XML-RPC requests are suppoed to be POST requests, not GET. By jvandyk and myself. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ April 29, 2004 - 19:15 : killes(a)www.drop.org Drupal includes xmlrpc.inc for every non-cached page view. However, the functions from the file are only rarely used. Therefore we need a patch that includes the file from the functions that want to use it. ------------------------------------------------------------------------ May 2, 2004 - 19:30 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc.patch (4.64 KB) Here is a patch that moves the include statement to the functiosn where xmlrpc.inc is actually needed. The patch coul dneed some testing as I do not use those modules (ping, bloggerapi) myself. ------------------------------------------------------------------------ May 3, 2004 - 22:34 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_0.patch (6.38 KB) Patch updated, I had forgotten drupal.module. thanks to crschmidt for spotting this. ------------------------------------------------------------------------ May 3, 2004 - 23:17 : Steven drupal_xml_parser_create() does not require xmlrpc.inc... ------------------------------------------------------------------------ May 3, 2004 - 23:27 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_1.patch (6.06 KB) Ok, updated. ------------------------------------------------------------------------ May 3, 2004 - 23:41 : Dries If xmlrpc.php includes xmlrpc.inc, why do we have to include it in blogapi module's functions once more? Also, the inclusion of xmlrpc.inc in drupal.module can probably be avoided by including xmlrpc.inc in cron.php? Am I missing something? ------------------------------------------------------------------------ May 3, 2004 - 23:49 : crschmidt Not all of the tasks in drupal.module are run with cron.php: for example, the distributed authentication comes in through drupal.module, which requires code from xmlrpc.inc to create the messages it sends out, as far as I can tell. So, the drupal.module needs it for distributed authentication stuff, not just for pinging servers and so on. ------------------------------------------------------------------------ May 4, 2004 - 00:38 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_2.patch (2.8 KB) "If xmlrpc.php includes xmlrpc.inc, why do we have to include it in blogapi module's functions once more?" As I said, I am not that familiar with that part of Drupal, did not investigate under whch circumstances blogapi functions could be called, and wanted to stay on the safe side. If you are sure that blogapi is always called from xmlrpcs.php, then apply the appended version. ------------------------------------------------------------------------ May 4, 2004 - 06:40 : Dries I'm not 100% sure either, I was merely suggesting an alternative. I think the second patch might work tough, in which case it would be preferred for its simplicity. Not? I'll add "Testing Gerhard's blog API patch to my short term TODO list". Hopefully I get to it tonight after work. Thanks Gerhard. ------------------------------------------------------------------------ May 12, 2004 - 21:12 : Kjartan Good patch, just a few minor comments. - Use single quotes for constant strings. - I noticed some indentations that were off the 2 column, looks like the are in the current code though. - drupal_directory_ping() will only be invoked from xmlrpc.php so there shouldn't be a need to include the file there. ------------------------------------------------------------------------ May 12, 2004 - 23:40 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_2_0.patch (2.64 KB) Updated. Don't know to which spaces you are referring. Guess Drupal CVS needs to be code checked again. ------------------------------------------------------------------------ August 4, 2004 - 02:39 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/xmlrpc_3.patch (2.81 KB) Updated again. It appears that parts of the old patch made it into drupal.module, but the include statement was still in common.inc. ------------------------------------------------------------------------ August 4, 2004 - 06:47 : TDobes +1... I tested the patch with blogapi.module and w.bloggar and it works fine, as far as I can tell. I like any patch that includes less code on each page view. ------------------------------------------------------------------------ August 4, 2004 - 21:36 : Dries Committed to HEAD. Thanks. ------------------------------------------------------------------------ August 13, 2004 - 19:47 : walkah Attachment: http://drupal.org/files/issues/xmlrpc-include.diff (2.38 KB) this patch didn't break blogapi.module, it's true. but it _did_ break drupal.module's distributed authentication feature. the problem is that xmlrpc.inc declares some classes, which will not work if the include statement appears inside a function. see the following for more details: http://us4.php.net/manual/en/language.functions.php (specifically the comment by albaity at php4web dot com) the attached patch restores the drupal.module functionality (as well as reintroduces the include in common.inc). I'm afraid that's just the way it has to be, sorry killes ;) (this patch also fixes a typo in drupal.module where the 'name' array key was not quoted - which may break things under higher error reporting levels). ------------------------------------------------------------------------ August 13, 2004 - 20:23 : Anonymous Can't you include common.inc from drupal.module if it needs that file? locale.module also includes its locale.inc without punishing the user who doesn't care about its functionality. Gerhard ------------------------------------------------------------------------ August 14, 2004 - 07:29 : Dries Including the XML-RPC library at the top of those files/modules that need it sounds like a fair alternative. ------------------------------------------------------------------------ August 14, 2004 - 12:19 : Steven But that doesn't really help unless you have no XML-RPC modules enabled at all... ------------------------------------------------------------------------ August 14, 2004 - 13:58 : JonBob What is more, I don't think it even fixes the bug. The problem is that class declarations in PHP4 are not guaranteed to work correctly when made inside a function. This applies to declarations immediately inside a function definition, or in files included from inside a function definition, or even from files included from files included from function definitions. Note that since module files are included from module_load(), this means class definitions or includes of files that have them cannot safely be in modules. Basically, conditional inclusion of classes is a no-no. To get around this we'd have to rewrite xmlrpc.inc using functions rather than object methods. ------------------------------------------------------------------------ August 16, 2004 - 19:29 : Dries I reverted the patch. ------------------------------------------------------------------------ August 17, 2004 - 16:39 : walkah ya, but you forgot to put the include_once back in common.inc . :) ------------------------------------------------------------------------ August 17, 2004 - 18:59 : killes(a)www.drop.org Can't just somebody step forward and rewrite that class in functions? ------------------------------------------------------------------------ August 31, 2004 - 22:41 : killes(a)www.drop.org I'd like to keep this open. It is so nicely assigned. ;-> ------------------------------------------------------------------------ September 1, 2004 - 13:35 : Anonymous *grumble* -- View: http://drupal.org/node/7458 Edit: http://drupal.org/project/comments/add/7458

1 0

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

31 Jan '05

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

1 0