Posted by boombatower on August 6, 2010 at 2:06am
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | major |
| Assigned: | sun |
| Status: | needs review |
| Issue tags: | Issue summary initiative, needs backport to D7 |
Issue Summary
I cannot use debug() or see the notice associated with
<?php
$i = 1 / 0;
?>from any code run on the XML-RPC side.
This also prevents me from implementing #639608: Provide assertXmlrpc() function to inspect and verify XML-RPC messages and is bad for tests in general. I think this should be considered at minimum a major issue since we are ignoring all kinds of errors in XML-RPC base system and hooks (mainly contrib there). Since it doesn't block using Drupal itself...not critical.
I'm investigating at the moment.
Comments
#1
Had some company...came back...looks to be solved.
The following patch add HTTP header error catching like DrupalWebTestCase::drupalGet() has to drupal_http_request() which is also used by xmlrpc(). So this is actually a more general fix. No test included with this...I can whip one up or we can use the test that will be included with #639608: Provide assertXmlrpc() function to inspect and verify XML-RPC messages which is built on this functionality.
#2
more accurate title and component
#3
The last submitted patch, 875342-http-request-errors.patch, failed testing.
#4
Fixed some conflicts.
#5
The last submitted patch, 875342-http-request-errors.patch, failed testing.
#6
Very interesting case of:
Test makes GET request
page that was requested makes GET request
end page has errors that are forwarded back
So in the middle case we need to realize we are not the parent and thus have no DrupalTestCase instance to call...so instead forward them back to the parent. CRAZINESS!
#7
I don't see a bug report here, so I can only guess this is a task.
Trying to make sense of the major queue currently. Given the very limited information, I don't see a reason for why this issue should be major.
#8
This makes testing XML-RPC related code or any other areas in contrib that use drupal_http_request() miss fatal errors (or any error for that matter). I really think we should look at getting this in. drupalGet|Post|etc pick this up in DrupalWebTestCase, but drupal_http_request() does not. This is the only area of core functionality (that I am aware of) that does not pick up errors as such as thus seems inconsistent and "dangerous" as far as testing is concerned.
This doesn't block core, but makes a ton of sense for contrib.
#9
<?phpcall_user_func_array(array(&DrupalTestCase::$self, 'error'), unserialize(urldecode($value)));
?>
This feels very insecure. I would simply re-trigger the errors, but we have to check if we are in a simpletest environment first.
#10
We need at the minimum a check for
!empty($test_info['test_run_id']) here.#11
Sounds like we should fix this, but no reason for it to block a RC.
#12
This needs to be fixed in d8 first.
#13
Tagging issues not yet using summary template.
#14
I'm maintaining a module that performs HTTP requests from the testing child-site to the testing child-site itself, so I'm highly interested in this, as I've wasted several hours with debugging already.
@Damien Tournoud's suggestion is spot on, exactly what I wanted to recommend as well. However, in my tests, the passed-through header wasn't always picked up (or rather returned).
Another issue with the approach is that the assertion headers are numbered, so we'd have to figure out the proper next number to pass on in order to not overwrite existing headers or potentially lose the new ones.
Therefore, attached patch implements a variant of that, which picks up the assertion headers from the response, and passes them through by triggering corresponding errors, which are then turned into HTTP assertion headers again.
This works excellently based on my testing.
#15
The last submitted patch, drupal.test-child-assertions.14.patch, failed testing.
#16
Meanwhile, I figured out why the direct pass-through of HTTP headers didn't work in all cases:
Some of my tests don't invoke drupalGet()/drupalPost() to the child-site, but instead are unit-test-alike testing the responses delivered from the child-site by calling drupal_http_request() indirectly. Thus, in that case, headers would be set for the parent site that is currently running the batch.
The two exceptions are caused by "Unknown errors" in UpdateTestContribCase. Attached patch accounts for "Unknown errors" and sets E_USER_ERROR instead. However, I'm actually not sure whether it is correct to hide them -- what the eff' is triggering a unknown error?
#17
The last submitted patch, drupal.test-child-assertions.16.patch, failed testing.
#18
well, looks like some non-E_USER_* errors are triggered there - so let's unconditionally trigger E_USER_ERROR.
This also means we have some real errors in that test.
#19
The last submitted patch, drupal.test-child-assertions.18.patch, failed testing.
#20
So these are actual errors in core. #1125662: UpdateTestContribCase fails on sites with contrib modules/themes in the filesystem might be the corresponding existing issue for those, but not sure.
#21
Actually, studying the code of update_test.module, those errors only pertain to testing and can be safely omitted/fixed. Attached patch does that.
#22
Agreed. Using @readfile would have sufficed as well.
#23
I guess this cannot be tested without "expected failure" functionality in SimpleTest.
#24
Re-rolled against latest branch HEAD.
#25
#24: drupal8.test-child-assertions.23.patch queued for re-testing.