Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#35185 closed defect (bug) (fixed)

Unable to create Post via XMLRPC after upgrading to 4.4

Reported by: sandeepprakash's profile Sandeep.Prakash Owned by: dd32's profile dd32
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: XML-RPC Keywords: has-patch needs-testing
Focuses: Cc:

Description

Unable to create a post via XMLRPC.
This was working flawless in WordPress 4.3.1

Here is the perl script to used to create a post.

   use WordPress::API::Post;

   my $p = WordPress::API::Post->new({
      username => 'UserName',
      password => 'Password',
      proxy => 'http://Sandeep-Prakash.com/xmlrpc.php'
   });

   $p->title('Hello World');
   $p->description('Testing the XML-RPC');

   $p->save; # save to wordpress

   $p->id;
   $p->url;

Attachments (2)

35185.diff (26.7 KB) - added by dd32 9 years ago.
number of places isset() would have to change to !empty() UNTESTED - Pretty sure I missed a lot of other $args[3] & $args[4] uses.
35185.2.diff (5.4 KB) - added by dd32 9 years ago.

Download all attachments as: .zip

Change History (20)

#1 @Sandeep.Prakash
9 years ago

If I am using the "Administrator" to create the Post above, then this is the error I get:

cant get id on saving, # WordPress::XMLRPC::newPost() - ERROR faultCode, 404
# WordPress::XMLRPC::newPost() - ERROR faultString, Invalid author ID.
 at /usr/local/share/perl/5.20.2/WordPress/Base/Object.pm line 134.
        WordPress::Base::Object::save(WordPress::API::Post=HASH(0xa21c18)) called at testPost.pl line 12

If I am using any other user (capable of creating post), here is the error I get:

cant get id on saving, # WordPress::XMLRPC::newPost() - ERROR faultCode, 401
# WordPress::XMLRPC::newPost() - ERROR faultString, You are not allowed to create posts as this user.
 at /usr/local/share/perl/5.20.2/WordPress/Base/Object.pm line 134.
        WordPress::Base::Object::save(WordPress::API::Post=HASH(0x1f55c18)) called at testPost.pl line 12

#2 @dd32
9 years ago

  • Milestone changed from Awaiting Review to 4.4.1
  • Owner set to dd32
  • Status changed from new to reviewing

Moving for review.

#3 follow-up: @dd32
9 years ago

  • Owner dd32 deleted

I've tested wp.newPost using the PHP client and can't see any issue. I don't have perl configured properly to test with the script provided here.

@Sandeep.Prakash it would be appreciated if you could provide the raw HTTP requests so that we're able to determine where the problem originates.

#4 in reply to: ↑ 3 @Sandeep.Prakash
9 years ago

@dd32 : Thanks for thequick response. Here is the tcpdump. NOTE: I've replaced the user name (USER-NAME)and password (PASSWORD).

12:55:33.344291 IP 192.168.1.2.57044 > cp-in-4.webhostbox.net.http: Flags [S], seq 3142370791, win 29200, options [mss 1460,sackOK,TS val 246544 ecr 0,nop,wscale 7], length 0
E..<..@.@..`....g.;....P.L........r.d..........
............
12:55:33.494357 IP 192.168.1.2.57044 > cp-in-4.webhostbox.net.http: Flags [.], ack 3013320556, win 229, options [nop,nop,TS val 246559 ecr 1945382366], length 0
E..4..@.@..g....g.;....P.L.....l....d......
....s.-.
12:55:33.706753 IP 192.168.1.2.57044 > cp-in-4.webhostbox.net.http: Flags [.], seq 0:1424, ack 1, win 229, options [nop,nop,TS val 246580 ecr 1945382366], length 1424: HTTP: POST /xmlrpc.php HTTP/1.1
E.....@.@.......g.;....P.L.....l....j......
...4s.-.POST /xmlrpc.php HTTP/1.1
TE: deflate,gzip;q=0.3
Connection: TE, close
Accept: text/xml
Accept: multipart/*
Accept: application/soap
Host: sandeep-prakash.com
User-Agent: SOAP::Lite/Perl/1.17
Content-Length: 1366
Content-Type: text/xml

<?xml version="1.0" encoding="UTF-8"?><methodCall><methodName>metaWeblog.newPost</methodName><params><param><value><int>1</int></value></param><param><value><string>USER-NAME</string></value></param><param><value><string>PASSWORD</string></value></param><param><value><struct><member><name>wp_password</name><value /></member><member><name>permaLink</name><value /></member><member><name>mt_keywords</name><value /></member><member><name>mt_text_more</name><value /></member><member><name>dateCreated</name><value /></member><member><name>link</name><value /></member><member><name>wp_slug</name><value /></member><member><name>mt_allow_comments</name><value /></member><member><name>date_created_gmt</name><value /></member><member><name>mt_excerpt</name><value /></member><member><name>title</name><value><string>Wonderful Thing</string></value></member><member><name>description</name><value><string>This is the main page content</string></value></member><member><name>wp_author_display_name</name><value /></member><member><name>wp_author_id</name><value /></member><member><name>mt_allow_pings</name><value /></member><member><name>categories</name><value
12:55:33.706983 IP 192.168.1.2.57044 > cp-in-4.webhostbox.net.http: Flags [P.], seq 1424:1614, ack 1, win 229, options [nop,nop,TS val 246580 ecr 1945382366], length 190: HTTP
E.....@.@.......g.;....P.L.x...l....eI.....
...4s.-./></member><member><name>userid</name><value /></member><member><name>postid</name><value /></member></struct></value></param><param><value><int>1</int></value></param></params></methodCall>
12:55:34.931943 IP 192.168.1.2.57044 > cp-in-4.webhostbox.net.http: Flags [.], ack 906, win 243, options [nop,nop,TS val 246703 ecr 1945383803], length 0
E..4..@.@..d....g.;....P.L.6........d......
....s.3{
12:55:34.938704 IP 192.168.1.2.57044 > cp-in-4.webhostbox.net.http: Flags [F.], seq 1614, ack 907, win 243, options [nop,nop,TS val 246703 ecr 1945383803], length 0
E..4..@.@..c....g.;....P.L.6........d......
....s.3{

Here is the payload extracted from above:

<?xml version="1.0" encoding="UTF-8"?>
<methodCall>
  <methodName>metaWeblog.newPost</methodName>
  <params>
    <param>
      <value>
        <int>1</int>
      </value>
    </param>
    <param>
      <value>
        <string>USER-NAME</string>
      </value>
    </param>
    <param>
      <value>
        <string>PASSWORD</string>
      </value>
    </param>
    <param>
      <value>
        <struct>
          <member>
            <name>wp_password</name>
            <value/>
          </member>
          <member>
            <name>permaLink</name>
            <value/>
          </member>
          <member>
            <name>mt_keywords</name>
            <value/>
          </member>
          <member>
            <name>mt_text_more</name>
            <value/>
          </member>
          <member>
            <name>dateCreated</name>
            <value/>
          </member>
          <member>
            <name>link</name>
            <value/>
          </member>
          <member>
            <name>wp_slug</name>
            <value/>
          </member>
          <member>
            <name>mt_allow_comments</name>
            <value/>
          </member>
          <member>
            <name>date_created_gmt</name>
            <value/>
          </member>
          <member>
            <name>mt_excerpt</name>
            <value/>
          </member>
          <member>
            <name>title</name>
            <value>
              <string>Wonderful Thing</string>
            </value>
          </member>
          <member>
            <name>description</name>
            <value>
              <string>This is the main page content</string>
            </value>
          </member>
          <member>
            <name>wp_author_display_name</name>
            <value/>
          </member>
          <member>
            <name>wp_author_id</name>
            <value/>
          </member>
          <member>
            <name>mt_allow_pings</name>
            <value/>
          </member>
          <member>
            <name>categories</name>
            <value/>
          </member>
          <member>
            <name>userid</name>
            <value/>
          </member>
          <member>
            <name>postid</name>
            <value/>
          </member>
        </struct>
      </value>
    </param>
    <param>
      <value>
        <int>1</int>
      </value>
    </param>
  </params>
</methodCall>

I see that it is using the method metaWeblog.newPost and not wp.newPost.

Last edited 9 years ago by Sandeep.Prakash (previous) (diff)

#5 @dd32
9 years ago

I see that it is using the method metaWeblog.newPost and not wp.newPost.

Thanks! That will hopefully make it reproducible :)

#6 follow-ups: @dd32
9 years ago

This is caused by [35509] #16980 cc @wonderboymusic

In short; mw_newPost would previously receive:

Array
(
    [title] => Wonderful Thing
    [description] => This is the main page content
)

it now receives:

Array
(
    [wp_password] =>
    [permaLink] =>
    [mt_keywords] =>
    [mt_text_more] =>
    [dateCreated] =>
    [link] =>
    [wp_slug] =>
    [mt_allow_comments] =>
    [date_created_gmt] =>
    [mt_excerpt] =>
    [title] => Wonderful Thing
    [description] => This is the main page content
    [wp_author_display_name] =>
    [wp_author_id] =>
    [mt_allow_pings] =>
    [categories] =>
    [userid] =>
    [postid] =>
)

The null/empty values were previously stripped out, which I can understand would be the expected behaviour.

With this change, it looks like every instance of isset( $content_struct[..] ) needs to be changed to ! empty( $content_struct[...] ).

I could also argue that the XMLRPC client shouldn't be sending those fields if it doesn't intend to specify a value, which means the WordPress::API CPAN module needs updating.

#7 in reply to: ↑ 6 @Sandeep.Prakash
9 years ago

The null/empty values were previously stripped out, which I can understand would be the expected behaviour.

With this change, it looks like every instance of isset( $content_struct[..] ) needs to be changed to ! empty( $content_struct[...] ).

I could also argue that the XMLRPC client shouldn't be sending those fields if it doesn't intend to specify a value, which means the WordPress::API CPAN module needs updating.

@dd32 Thank You for investing your time into the analysis of this issue.

I see that the milestone has been set to 4.4.1, which means that the change would be coming in the v4.4.1.
As a work around till the v4.4.1 goes live (which is expected in April) I will format the XML myself and POST it via cURL.

I do to some extent agree to your argument that XMLRPC shouldn't be sending the empty fields. But on the other hand, going as per the general standards, an application should be able to accept null values if it's not mandatory field and perform the needed checking at it's end. But this is just my understanding, I may be wrong too.

I will raise a bug on the WordPress::API tracker, so that it can be improvised. Also, sending empty fields is always an overhead.

Once again thanks for the quick response and analysis.

#8 @dd32
9 years ago

I've set the milestone to 4.4.1, but it'll only be included if there's a resolution before then & someone to step up and fix it.

It may be that we simply should treat empty strings for some fields as unset (which I think is the case) but that's going to need someone to audit every XML-RPC endpoint.. or it may be decided that supplying an empty field is invalid for a client to do so, in which case there may not be any fix in 4.4.1.

The other option is to simply revert [35509] and attempt another review or iteration on that in 4.5 instead.

Last edited 9 years ago by dd32 (previous) (diff)

#9 @dd32
9 years ago

The other option is to simply revert [35509] and attempt another review or iteration on that in 4.5 instead.

This is my preferred option. I'll attach a patch of how many places in our XML class expects empty elements not to be set shortly.

@dd32
9 years ago

number of places isset() would have to change to !empty() UNTESTED - Pretty sure I missed a lot of other $args[3] & $args[4] uses.

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


9 years ago

#11 in reply to: ↑ 6 @dossy
9 years ago

Replying to dd32:

The null/empty values were previously stripped out, which I can understand would be the expected behaviour.

With this change, it looks like every instance of isset( $content_struct[..] ) needs to be changed to ! empty( $content_struct[...] ).

I could also argue that the XMLRPC client shouldn't be sending those fields if it doesn't intend to specify a value, which means the WordPress::API CPAN module needs updating.

I'd like to say that there is a semantic difference between a struct element not existing and not having any value, vs. it existing and its value is null, as well as its value being empty but not null.

Unfortunately, the nil value is an XML-RPC extension, so I wouldn't expect to support it unless we intend to support that extension. The standard convention is to represent NULL values as just that, a <value> element with no data, e.g. <value/> or <value></value> which is what @Sandeep.Prakash's tcpdump output shows is coming across.

Not including those keys in the struct is different than being included in the struct but having a NULL value. Being able to test for an array_key_exists() to test if it was included in the struct is useful and being able to test it for is_null() ...

It's unfortunate that r35509 actually implements "better" support for XML-RPC, at the cost of breaking everything that sits behind it, which doesn't differentiate between struct members not existing vs. them having a NULL value.

@dd32
9 years ago

#12 @dd32
9 years ago

It's unfortunate that r35509 actually implements "better" support for XML-RPC, at the cost of breaking everything that sits behind it, which doesn't differentiate between struct members not existing vs. them having a NULL value.

I'm still not 100% sure on the way forward here, but here's the several use-cases which we have:

  • <value /> and <value></value> (which are identical to the XML parser)
  • <value><string /></value> and <value><string></string></value>
  • <value>text</value> which is the same as <value><string>text</string></value>

Here's one interpretation:

  • <value /> and <value></value> = NULL - isset() = false, array_key_exists() = true, will restore the 4.3 behaviour, those which need/want to detect the "empty" tag can do so.
  • <value><string /></value> and <value><string></string></value> = '' (empty string), if any clients are specifically specifying an empty string here, we're going to assume that's what they actually want to do
  • <value>text</value> & <value><string>text</string></value> = same behaviour, both are treated as a string.

35185.2.diff implements the above, and fixes the MT XML-RPC endpoints to use it.

#13 @dossy
9 years ago

LGTM. Does this address the bug that was originally being reported, too?

#14 @dossy
9 years ago

  • Keywords has-patch needs-testing added

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


9 years ago

#16 @redsweater
9 years ago

It's common for servers and clients to exhibit quirks with respect to XMLRPC, and it's loosely specified enough that I don't know if you can even find consensus for how it should 100% behave. I would argue that the status quo trumps most other considerations and that changes to XMLRPC behavior in WordPress should only be made to achieve specific, tangible goals. I tend to think that [35509] should be reverted and that additional fixes to XMLRPC support should be considered skeptically, as they might cause similarly unexpected side-effects.

Last edited 9 years ago by redsweater (previous) (diff)

#17 @dd32
9 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#18 @dd32
9 years 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.