Closed (fixed)
Project:
Drupal core
Component:
base system
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
29 Apr 2004 at 18:15 UTC
Updated:
12 Oct 2005 at 22:20 UTC
Jump to comment: Most recent file
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.
| Comment | File | Size | Author |
|---|---|---|---|
| #34 | xmlrpc_comment_spaces.patch | 2.17 KB | chx |
| #33 | xmlrpc_cond_1.patch | 4.69 KB | chx |
| #32 | xmlrpc_cond_0.patch | 4.58 KB | chx |
| #31 | xmlrpc_cond.patch | 4.4 KB | chx |
| #26 | xmplrpc.patch | 695 bytes | killes@www.drop.org |
Comments
Comment #1
killes@www.drop.org commentedHere 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.
Comment #2
killes@www.drop.org commentedPatch updated, I had forgotten drupal.module. thanks to crschmidt for spotting this.
Comment #3
Steven commenteddrupal_xml_parser_create() does not require xmlrpc.inc...
Comment #4
killes@www.drop.org commentedOk, updated.
Comment #5
dries commentedIf
xmlrpc.phpincludesxmlrpc.inc, why do we have to include it in blogapi module's functions once more? Also, the inclusion ofxmlrpc.incindrupal.modulecan probably be avoided by includingxmlrpc.incincron.php? Am I missing something?Comment #6
crschmidt commentedNot 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.
Comment #7
killes@www.drop.org commented"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.
Comment #8
dries commentedI'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.
Comment #9
Kjartan commentedGood 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.
Comment #10
killes@www.drop.org commentedUpdated. Don't know to which spaces you are referring. Guess Drupal CVS needs to be code checked again.
Comment #11
killes@www.drop.org commentedUpdated again. It appears that parts of the old patch made it into drupal.module, but the include statement was still in common.inc.
Comment #12
TDobes commented+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.
Comment #13
dries commentedCommitted to HEAD. Thanks.
Comment #14
walkah commentedthis 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).
Comment #15
(not verified) commentedCan'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
Comment #16
dries commentedIncluding the XML-RPC library at the top of those files/modules that need it sounds like a fair alternative.
Comment #17
Steven commentedBut that doesn't really help unless you have no XML-RPC modules enabled at all...
Comment #18
jonbob commentedWhat 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.
Comment #19
dries commentedI reverted the patch.
Comment #20
walkah commentedya, but you forgot to put the include_once back in common.inc . :)
Comment #21
killes@www.drop.org commentedCan't just somebody step forward and rewrite that class in functions?
Comment #22
dries commentedComment #23
(not verified) commentedComment #24
killes@www.drop.org commentedI'd like to keep this open. It is so nicely assigned. ;->
Comment #25
(not verified) commented*grumble*
Comment #26
killes@www.drop.org commentedHere is an alternative approach. XML-RPC requests are suppoed to be POST requests, not GET. By jvandyk and myself.
Comment #27
jvandyk commentedThis 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.
Comment #28
jvandyk commentedAnd 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.
Comment #29
tangent commentedIt seems that implementing xmlrpc as a (required) module would work around the include issue. I'm working on a xmlrpc.module from scratch, sort of as a Drupal learning exercise, which uses the XML-RPC extension functions instead of the xmlrpc.inc that we're using now and will reduce the code to a few lines instead of the big class that exists now.
Before I get too far into it though, I'm interested if anyone may have thoughts about why a module is not the way to go, or if there is some reason we shouldn't be using the XML-RPC functions.
Comment #30
moshe weitzman commentedComment #31
chx commentedThe new library lets this happen. Also, this patch unifies the xmlrpc() and xmlrpc_multicall() calls. Most sites now load 12K less PHP code for almost all page requests.
Comment #32
chx commentedDries asked for more clear phpdoc. Robert Douglass and I have enhanced it.
Comment #33
chx commentedSteven found that the faster ./ is missing from xmlrpc.php
Comment #34
chx commentedDries commited this but something went foobar in the latest two patches and now my phpdoc is broken :( only changes are in the comment.
Comment #35
dries commentedCommitted to HEAD.
Comment #36
(not verified) commentedComment #37
(not verified) commentedComment #38
(not verified) commentedComment #39
(not verified) commented