[drupal-devel] [task] Rewrite to use procedures and conditionally
include xmlrpc.inc
chx
drupal-devel at drupal.org
Tue Aug 16 17:51:20 UTC 2005
Issue status update for
http://drupal.org/node/7458
Post a follow up:
http://drupal.org/project/comments/add/7458
Project: Drupal
Version: cvs
Component: base system
Category: tasks
Priority: normal
Assigned to: chx
Reported by: killes at www.drop.org
Updated by: chx
Status: patch (ready to be committed)
Attachment: http://drupal.org/files/issues/xmlrpc_cond_0.patch (4.58 KB)
Dries asked for more clear phpdoc. Robert Douglass and I have enhanced
it.
chx
Previous comments:
------------------------------------------------------------------------
Thu, 29 Apr 2004 18:15:33 +0000 : killes at 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.
------------------------------------------------------------------------
Sun, 02 May 2004 18:30:55 +0000 : killes at 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.
------------------------------------------------------------------------
Mon, 03 May 2004 21:34:39 +0000 : killes at 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.
------------------------------------------------------------------------
Mon, 03 May 2004 22:17:52 +0000 : Steven
drupal_xml_parser_create() does not require xmlrpc.inc...
------------------------------------------------------------------------
Mon, 03 May 2004 22:27:55 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/xmlrpc_1.patch (6.06 KB)
Ok, updated.
------------------------------------------------------------------------
Mon, 03 May 2004 22:41:53 +0000 : 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?
------------------------------------------------------------------------
Mon, 03 May 2004 22:49:58 +0000 : 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.
------------------------------------------------------------------------
Mon, 03 May 2004 23:38:35 +0000 : killes at 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.
------------------------------------------------------------------------
Tue, 04 May 2004 05:40:48 +0000 : 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.
------------------------------------------------------------------------
Wed, 12 May 2004 20:12:10 +0000 : 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.
------------------------------------------------------------------------
Wed, 12 May 2004 22:40:06 +0000 : killes at 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.
------------------------------------------------------------------------
Wed, 04 Aug 2004 01:39:34 +0000 : killes at 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.
------------------------------------------------------------------------
Wed, 04 Aug 2004 05:47:45 +0000 : 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.
------------------------------------------------------------------------
Wed, 04 Aug 2004 20:36:43 +0000 : Dries
Committed to HEAD. Thanks.
------------------------------------------------------------------------
Fri, 13 Aug 2004 18:47:36 +0000 : 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).
------------------------------------------------------------------------
Fri, 13 Aug 2004 19:23:28 +0000 : 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
------------------------------------------------------------------------
Sat, 14 Aug 2004 06:29:36 +0000 : Dries
Including the XML-RPC library at the top of those files/modules that
need it sounds like a fair alternative.
------------------------------------------------------------------------
Sat, 14 Aug 2004 11:19:50 +0000 : Steven
But that doesn't really help unless you have no XML-RPC modules enabled
at all...
------------------------------------------------------------------------
Sat, 14 Aug 2004 12:58:52 +0000 : 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.
------------------------------------------------------------------------
Mon, 16 Aug 2004 18:29:40 +0000 : Dries
I reverted the patch.
------------------------------------------------------------------------
Tue, 17 Aug 2004 15:39:25 +0000 : walkah
ya, but you forgot to put the include_once back in common.inc . :)
------------------------------------------------------------------------
Tue, 17 Aug 2004 17:59:05 +0000 : killes at www.drop.org
Can't just somebody step forward and rewrite that class in functions?
------------------------------------------------------------------------
Tue, 31 Aug 2004 21:41:24 +0000 : killes at www.drop.org
I'd like to keep this open. It is so nicely assigned. ;->
------------------------------------------------------------------------
Wed, 01 Sep 2004 12:35:31 +0000 : Anonymous
*grumble*
------------------------------------------------------------------------
Mon, 31 Jan 2005 23:05:37 +0000 : killes at 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.
------------------------------------------------------------------------
Mon, 31 Jan 2005 23:22:18 +0000 : 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.
------------------------------------------------------------------------
Mon, 31 Jan 2005 23:24:23 +0000 : jvandyk
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.
------------------------------------------------------------------------
Tue, 01 Feb 2005 03:51:54 +0000 : tangent
It 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.
------------------------------------------------------------------------
Tue, 16 Aug 2005 13:46:41 +0000 : chx
Attachment: http://drupal.org/files/issues/xmlrpc_cond.patch (4.4 KB)
The 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.
More information about the drupal-devel
mailing list