Sub-task of #1862398: [meta] Replace drupal_http_request() with Guzzle

CommentFileSizeAuthor
#13 xml-rpc-guzzle-1875614-13.patch3.89 KBYesCT
#13 interdiff-10-13.txt1.08 KBYesCT
#10 xml-rpc-guzzle-1875614-10.patch3.86 KBBerdir
#2 xmlrpc-guzzle-1862512-2.patch3.53 KBBerdir
#2 xmlrpc-guzzle-1862512-2-interdiff.txt603 bytesBerdir
#1 xmlrpc-guzzle-1862512-1.patch3.48 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
xmlrpc-guzzle-1862512-1.patch3.48 KB

Seems to be easy enough.

Changed $options to $headers, not an exact conversion but not sure what other options you'd might want to set. Not much possible without refactoring the whole module...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
xmlrpc-guzzle-1862512-2-interdiff.txt603 bytes
xmlrpc-guzzle-1862512-2.patch3.53 KB

We can't rely on $response, see #1862524: Convert drupal_http_request usage in aggregator.module to Guzzle

Status: Needs review » Needs work

The last submitted patch, xmlrpc-guzzle-1862512-2.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

#2: xmlrpc-guzzle-1862512-2.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, xmlrpc-guzzle-1862512-2.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

#2: xmlrpc-guzzle-1862512-2.patch queued for re-testing.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

#2: xmlrpc-guzzle-1862512-2.patch queued for re-testing.

mitron’s picture

mitron CreditAttribution: mitron commented

This patch changes the xmlrpc api. Is that what is intended? There are no calls or tests currently in core that depend on the current api so the tests will not show any problems with this api change. Do we care if anything is being broken in contrib?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

We don't care, API changes can still happen.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
xml-rpc-guzzle-1875614-10.patch3.86 KB

Re-roll, using the improved exceptions and Drupal::httpClient().

The XML-RPC library needs quite a bit of work but I'm trying to keep the patch size down. But the whole thing should probably be a service and get the http client injected, throw exception and stuff like that.

YesCT’s picture

YesCT CreditAttribution: YesCT commented 
+++ b/core/modules/xmlrpc/xmlrpc.incundefined
@@ -526,15 +529,15 @@ function xmlrpc_base64_get_xml($xmlrpc_base64) {
- * @param $args
+ * @param array $args
...
+ * @param array $headers
+ *   (optional) An array of HTTP headers to pass along.

@@ -543,7 +546,7 @@ function xmlrpc_base64_get_xml($xmlrpc_base64) {
+function _xmlrpc($url, $args, $headers = array()) {

+++ b/core/modules/xmlrpc/xmlrpc.moduleundefined
@@ -62,7 +62,7 @@ function xmlrpc_menu() {
+function xmlrpc($url, $args, $headers = array()) {

do we want to type hint array in the function definition?

like:
function _xmlrpc($url, array $args, array $headers = array())

otherwise, standards-wise this looks good.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Sure, can do that :)

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
interdiff-10-13.txt1.08 KB
xml-rpc-guzzle-1875614-13.patch3.89 KB

related: #1158720: Improve text for parameter type hinting in function declaration

here is the patch to add the type hinting.

Status: Needs review » Needs work

The last submitted patch, xml-rpc-guzzle-1875614-13.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented


Invalid values generate a list of form errors. Other EntityTranslationUITest.php 40 Drupal\translation_entity\Tests\EntityTranslationUITest->testTranslationUI()

Seen that one a lot recently, in various translation UI tests. How can @plach write that many randomly failing tests ;)

Needs an issue.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

#13: xml-rpc-guzzle-1875614-13.patch queued for re-testing.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

Here is the new issue:
#1965048: Frequent random test failure EntityTranslationUITest Invalid values generate a list of form errors.
for #15.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

Looks like a pretty straight conversion.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Thanks. There is one small API change here, instead of allowing arbitrary drupal_http_request() options, we just accept an array of headers now. So that will need a small change notice but I have no idea if anyone out there is actually using that :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Convert drupal_http_request usage in xmlrpc.module to Guzzle » Change notice: Convert drupal_http_request usage in xmlrpc.module to Guzzle
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed a51bb7e and pushed to 8.x. Thanks!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Change notice: Convert drupal_http_request usage in xmlrpc.module to Guzzle » Convert drupal_http_request usage in xmlrpc.module to Guzzle
Priority: Critical » Normal
Status: Active » Fixed

Created http://drupal.org/node/1968052

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: -Needs change record

And removing tag. I could swear I did that above.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.