In relation to #1842574: Warning in tests, servicesDelete also seems to mess up the following requests, as it uses the CURLOPT_CUSTOMREQUEST setting, which sticks around to the next request and, for instance, makes servicesGet do weird stuff like returning a 200 HTTP code and an empty result (which is most unexpected when you try to get the object just deleted, expecting a 404 error, or some content if the delete was ineffectual).

It can be fixed in the same way, but the question is whether it wasn't healthier to take the hint that Curl apparently wasn't build for reusing the same Curl instance for very different requests and instead always re-initializing a new Curl? What do you say Kyle?

CommentFileSizeAuthor
#9 services-reset_every_request-1860068-9.patch1.77 KBxen
#5 services-reset_every_request-1860068-5.patch1.8 KBxen
#4 services-delete_reset-1860068-4.patch1.63 KBxen
#4 services-reset_every_request-1860068-4.patch1.51 KBxen

Comments

marcingy’s picture

marcingy commented

Services builds off DrupalWebTestCase, I believe if there are issues with cURL they should be fixed upstream rather than in services itself especially if there is a general issue with cURL.

kylebrowning’s picture

kylebrowning commented

Itll probably make the tests last longer but Im open to it.

xen’s picture

xen commented

@marcingy: Ideally, yes... However, ServicesWebTestCase already overrides much of DrupalWebTestCase, in particular the rather core curlExec method. Furthermore, the issues seems to crop up when using the PUT and DELETE methods, which is, as far as I know, not used in core. As long as you stick with GET and POST, things seems to work out fine. So, one could make the argument that core DrupalWebTestCase isn't broken, if you take the viewpoint that it wasn't supposed to be a fully REST compatible testing class in the first place.

OTOH, ServicesWebTestCase seems to have originally overridden the curlExec method just to fix the log message showing the right method, so the question is whether it wouldn't be nicer to get it all upstream.

@kylebrowning
Yes, obviously, but I think the difference is microscopic. I think the runtime difference between test runs due to other factors will drown out the difference.

I'll whip up a patch tomorrow. I assume it could be backported to DrupalWebTestCase trivially, if that's desired.

xen’s picture

xen commented
Status: Active » Needs review
StatusFileSize
new services-reset_every_request-1860068-4.patch1.51 KB
new services-delete_reset-1860068-4.patch1.63 KB

Couldn't resist trying out both options.

xen’s picture

xen commented
StatusFileSize
new services-reset_every_request-1860068-5.patch1.8 KB

servicesDelete forgets to mention that it wants headers as part of the result. Added to this patch.

marcingy’s picture

marcingy commented

This patch looks nice and straight forward and actually much nicer than the code it is replacing :) I haven't tested the change but from a code prospect it looks good from me.

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Reviewed & tested by the community
ygerasimov’s picture

ygerasimov commented
Status: Reviewed & tested by the community » Needs work 
+++ b/tests/services.testundefined
@@ -2,7 +2,7 @@
 
-  protected $previousExecHadInfile = FALSE;
+  protected $resetCurl = FALSE;

If we reset our cURL handler without any condition we do not need $resetCurl variable at all.

So please remove it and patch is ready to be committed.

xen’s picture

xen commented
Status: Needs work » Needs review
StatusFileSize
new services-reset_every_request-1860068-9.patch1.77 KB

Oh, that's right. Fixed.

ygerasimov’s picture

ygerasimov commented
Status: Needs review » Fixed

Thank you. Committed.

Status: Fixed » Closed (fixed)

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