WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 7 months ago

#16980 reopened defect (bug)

Empty Values are ignored by class-ixr.php

Reported by: nprasath002 Owned by: wonderboymusic
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: XML-RPC Keywords: has-patch
Focuses: Cc:

Description (last modified by aaroncampbell)

I tried to fix the following bug #10599
Found out when you send and empty value via xmlrpc it converts it to null value.

Say you send and array of arguments for mw_editpost, set

$content_struct[mt_keywords] = '';


IXR client passes a null value instead of an empty value.

In mw_post method consider this statement

$tags_input = isset( $content_struct[mt_keywords] ) ? $content_struct[mt_keywords] : null;

Even if you send an empty value this statement fails because

$content_struct[mt_keywords]


is set to null by IXR client.

Attachments (1)

16980.diff (1.4 KB) - added by solarissmoke 5 years ago.

Download all attachments as: .zip

Change History (30)

#1 @nprasath002
5 years ago

I tried to fix the following bug
http://core.trac.wordpress.org/ticket/10599
Found out when you send and empty value via xmlrpc it converts it to null value.

Say you send and array of arguments for mw_editpost, set

$content_struct[mt_keywords] = '';


IXR client passes a null value instead of an empty value.

In mw_post method consider this statement

$tags_input = isset( $content_struct[mt_keywords] ) ? $content_struct[mt_keywords] : null;

Even if you send an empty value this statement fails because

$content_struct[mt_keywords]


is set to null by IXR client.

Last edited 5 years ago by nprasath002 (previous) (diff)

#2 follow-up: @ocean90
5 years ago

  • Description modified (diff)

Copied from the comment.

#3 in reply to: ↑ 2 @nprasath002
5 years ago

Replying to ocean90:

Copied from the comment.

I edited again. Fix that in the description

$tags_input = isset( $content_struct[mt_keywords] ) ? $content_struct[mt_keywords] : null;

P.S how can i edit the decription??

#4 @aaroncampbell
5 years ago

  • Description modified (diff)

Updated the code in description.

nprasath002: I'm not sure exactly how the permissions are set up, but I do know that not everyone can change a ticket description.

#5 @nprasath002
5 years ago

Related [10599]

Version 0, edited 5 years ago by nprasath002 (next)

#6 @solarissmoke
5 years ago

  • Keywords reporter-feedback added; needs-patch removed

I can't reproduce this. I used the following struct to edit an existing post (which had a bunch of tags):

<struct>
 <member>
  <name>title</name>
  <value><string>Hello world 2</string></value>
 </member>
 <member>
  <name>description</name>
  <value><string>My content</string></value>
 </member>
 <member>
  <name>mt_keywords</name>
  <value><string/></value>
 </member>
</struct>

i.e., mt_keywords is just an empty string. This worked fine - it removed all the tags from the post. Is your XML structure different? Please give us a structure that will reproduce the problem.

#7 @nprasath002
5 years ago

Hi
I tested with your structure and it works fine.
My structure is different.
It does'nt use datatypes.
I used Apache xml rpc libraries to generate this xml structure.
Don't know what others clients do.
What client you use?

Here is my structure.

<struct>
 <member>
  <name>title</name>
  <value>keyword change</value>
 </member>
 <member>
  <name>description</name>
  <value>check whether it is successful</value>
 </member>
 <member>
  <name>mt_keyword</name>
  <value></value>
 </member>
</struct>
Last edited 5 years ago by nprasath002 (previous) (diff)

#8 @nprasath002
5 years ago

The original problem says it uses windows live writer
#10599
I think live writer also don't use datatypes.
For me as i have used numerous xml-rpc calls everything worked fine
with my structure except setting this empty value over xmlrpc.

The problem here is we dont have a service description file to
strict the incoming messages. xmlrpc does not support WSDL files

Last edited 5 years ago by nprasath002 (previous) (diff)

#9 @solarissmoke
5 years ago

  • Keywords reporter-feedback removed

Reproduced it with your struct. The issue is that the XML parser is just ignoring an empty value tag <value></value>. This is a bug, because an empty value tag should be treated as an empty string according to http://www.xmlrpc.com/spec . I've got a patch working and will post it after a bit of testing.

Last edited 5 years ago by solarissmoke (previous) (diff)

#10 @solarissmoke
5 years ago

  • Keywords has-patch dev-feedback added

Patch fixes is for me. Logic is as follows:

  • According to the spec, the <value> tag can either contain a subtag which specifies the data type, or can contain a string. As it was, the parser would only handle non-empty strings so supplying <value></value> would cause it to just ignore the whole node.
  • My patch adds a tracking variable so we know when we reach the end of a <value> element whether it contained a sub-tag. If it didn't, we just use the contents of the <value> tag itself - empty or otherwise. Now the struct provided in comment:7 works.

PS Devs - what is the policy on modifying external libraries? I've reported the bug with Incutio in the hope they might address it in a future version, no response yet.

#11 @solarissmoke
5 years ago

  • Summary changed from Empty Values are converted to null by class-ixr.php to Empty Values are ignored by class-ixr.php

#12 @nprasath002
5 years ago

Hi,
Unfortunately this is not working.
Tested with a java client

@solarissmoke
5 years ago

#13 @nprasath002
5 years ago

  • Keywords needs-patch added; has-patch removed

#14 @nprasath002
5 years ago

  • Keywords has-patch added; needs-patch removed

#15 @nprasath002
5 years ago

Scratch that its working.

#16 @josephscott
5 years ago

It looks like this all hinges on "If no type is indicated, the type is string.". I'm not going to consider this critical for WP 3.2 at this point since there is a clear and obvious work around: always include a type.

#17 @solarissmoke
5 years ago

there is a clear and obvious work around: always include a type.

Most XML-RPC clients don't let you control that. You just supply them the data and they produce the XML, which in this case wordpress is handling incorrectly.

#18 follow-up: @nprasath002
5 years ago

Also specifications does not require to include a type

#19 in reply to: ↑ 18 @ericmann
5 years ago

Replying to nprasath002:

Also specifications does not require to include a type

Replying to solarissmoke:

in this case wordpress is handling incorrectly.

You're both partly right. The specification does require a type, but if no type is provided, <string /> is assumed. From the spec:

If no type is indicated, the type is string.

So yes, there is a problem that needs fixing ... but since there is a valid workaround (always include a type), it's not vital. It's good form to always send a type anyway ... but it's also good form to assume that the clients sending you data don't really know what they're doing (Be conservative in what you send, be liberal in what you accept.).

Now the question arises, is this a bug in WordPress' handling of XML-RPC? Or is this a bug inherent in the IXR library we're using? If it's an IXR issue, we should look at patching and fixing it with them ... otherwise, patch away here.

#20 @solarissmoke
5 years ago

is this a bug in WordPress' handling of XML-RPC? Or is this a bug inherent in the IXR library we're using?

It is a bug in the library, which completely ignores empty values that do not explicitly specify a type. I wrote to the folks at Incutio at the time that this ticket was opened. They wrote back saying that they would look into it - I've heard nothing back since.

Last edited 5 years ago by solarissmoke (previous) (diff)

#21 @chriscct7
22 months ago

  • Severity changed from major to normal

#22 @wonderboymusic
10 months ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 4.4

The library hasn't been updated since 2010, so our version, 1.7.4, is the latest #winning

If we're going to keep using it (can't imagine we wouldn't), we can proactively make fixes. Letting this sit for a bit though.

#23 @wonderboymusic
10 months ago

In 34682:

XML-RPC: add a unit test for mw.editPost.

See #16980.

#24 @wonderboymusic
9 months ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 35509:

XMLRPC: ensure that empty strings are not passed as null, which will then fail isset()

Props solarissmoke.
Fixes #16980.

#25 @dd32
7 months ago

This caused #35185 - XML-RPC endpoints have been using isset() on fields for quite some time, relying upon the fact that elements that weren't specified (or specified as empty) were being discarded.

It seems that at least one client, the WordPress::API CPAN module relied upon this behaviour, by sending all fields, even ones which were unspecified by the caller, to XML-RPC.

The fix appears to be that all XML RPC endpoints need to switch from isset() to !empty() as that's what their intentions are, but that's unlikely to happen in a point release, which means [35509] will be reverted unless there's a strong reason not to (or that sending empty fields, like is happening #35185, is invalid per spec), It'll need a champion to work on it for 4.5 inclusion then.

@solarissmoke @nprasath002 @ericmann I know that your comments are mostly 5 years old, but any chance you could take a glance at the above ticket?

This ticket was mentioned in Slack in #core by dossy. View the logs.


7 months ago

#27 @dd32
7 months ago

  • Milestone changed from 4.4 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reverting [35509] if someone would like to pick that up and possibly combine it with the patches in 12:ticket:35185 for 4.5 or later, please feel free.

Please be sure to include unit tests which show the behaviour before and after for all edge cases.

#28 @dd32
7 months ago

In 36132:

XMLRPC: Revert [35509] which caused a change of behviour in at least one XMLRPC client.

XMLRPC has many quirks in it's operation, #16980 being just one of the many, #35185 just became yet another quirk.

See #16980.
Fixes #35185.

#29 @dd32
7 months ago

In 36133:

XMLRPC: Revert [35509] which caused a change of behviour in at least one XMLRPC client.

XMLRPC has many quirks in it's operation, #16980 being just one of the many, #35185 just became yet another quirk.

Merges [36132] to the 4.4 branch.
See #16980.
Fixes #35185.

Note: See TracTickets for help on using tickets.