Files: 
CommentFileSizeAuthor
#18 aggregator-guzzle-1862524-18.patch1.87 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 53,915 pass(es).
[ View ]
#13 aggregator-guzzle-1862524-13.patch1.87 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 53,966 pass(es).
[ View ]
#12 aggregator-guzzle-1862524-12.patch2.17 KBSutharsan
PASSED: [[SimpleTest]]: [MySQL] 50,683 pass(es).
[ View ]
#6 aggregator-guzzle-1862524-6.patch2.16 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-guzzle-1862524-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 aggregator-guzzle-1862524-1.patch2.32 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,260 pass(es), 5 fail(s), and 2 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.32 KB
FAILED: [[SimpleTest]]: [MySQL] 49,260 pass(es), 5 fail(s), and 2 exception(s).
[ View ]

Conversion is easy, although the code in there is weird. There is no user facing error message when the opml download fails, just a green "nothing to do" message, but that's not my problem here:)

When doing manual testing, I noticed that we can't rely on $e->getResponse() because that might be NULL. If you enter a non-existing domain, then $e->getResponse()->getReasonPhrase() is a fatal error. Not sure what the recommended error handling is. It does throw an error on 404, at least, that much I've checked.

I suggest we also fix the DefaultFetcher issues in here, there are a lot of follow-ups already...

- Handling of 301 redirected URL
- Improve error handling to prevent fatal errors.

Status:Needs review» Needs work

The last submitted patch, aggregator-guzzle-1862524-1.patch, failed testing.

Guzzle throws an exception on any response code >= 400, and on curl transfer errors.

4xx errors throw a Guzzle\Http\Exception\ClientErrorResponseException
5xx errors throw a Guzzle\Http\Exception\ServerErrorResponseException
cURL errors throw a Guzzle\Http\Exception\CurlException

Does that help?

Yes. What I'm not sure is what's the best way to handle those errors in a generic way so we that can add a useful log message without having to duplicate a lot of code.

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -353,16 +354,18 @@ function aggregator_form_opml_submit($form, &$form_state) {
+      $error = $e->getResponse() ? $e->getResponse()->getReasonPhrase() : $e->getMessage();
+      watchdog('aggregator', 'HTTP request to @url failed with error: @error', array('@url' => $form_state['values']['remote'], '@error' => $error));

That's what I'm currently doing in this patch. getMessage(), especially in the case of Curl exceptions contain a lot debug information that isn't really useful for logging nor to provide as feedback in a UI.

Status:Needs work» Needs review
StatusFileSize
new2.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-guzzle-1862524-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Another one simplified a bit.

This looks good to me. The only semi-annoying thing is that the CurlException message contains a variable export that gets stuck in watchdog, but I guess that's ok for debugging purposes. If it is a problem, we could set up a cascading catch block and call ->getError() on the CurlException, which returns a more readable string.

Yes, we need to come up with a standard, useful way to handle and log guzzle exceptions.

Having two catch blocks is certainly cleaner than my getResponse() hack, although its a bit more code. Maybe something like $error = $e instanceof CurlException ? $e->getError() : $e->getResponse()->getReasonPhrase() ?

Or we could improve guzzle to provide a useful default exception message with getMessage()... that var_export() stuff on getMessage() is kinda weird anyway, never see anyone else doing that?

Ha. Funny timing. I just posted a possible solution here: #1875792: Standardize Guzzle exception handling

#6: aggregator-guzzle-1862524-6.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, aggregator-guzzle-1862524-6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 50,683 pass(es).
[ View ]

Patch rerolled.

StatusFileSize
new1.87 KB
PASSED: [[SimpleTest]]: [MySQL] 53,966 pass(es).
[ View ]

Re-roll, using the same pattern for adding an actually useful error that is displayed to the user. More or less same code as DefaultFetcher. Which had the reference fixed, so no longer part of the patch.

Status:Needs review» Needs work

The last submitted patch, aggregator-guzzle-1862524-13.patch, failed testing.

Status:Needs work» Needs review

#13: aggregator-guzzle-1862524-13.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

+1 for the return addition..no need to trigger _aggregator_parse_opml() if there are no data
rtbc'ing and i hope bot agrees:)

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/aggregator/aggregator.admin.inc
@@ -186,12 +188,22 @@ function aggregator_form_opml_submit($form, &$form_state) {
+      watchdog('aggregator', 'Faled to download OPML file due to "%error".', array('%error' => $response->getStatusCode() . ' ' . $response->getReasonPhrase()), WATCHDOG_WARNING);

Typo: Faled -> Failed

Applies to the other watchdog()s and dsm()s as well.

Status:Needs work» Needs review
StatusFileSize
new1.87 KB
PASSED: [[SimpleTest]]: [MySQL] 53,915 pass(es).
[ View ]

oops:)

Status:Needs review» Needs work

The last submitted patch, aggregator-guzzle-1862524-18.patch, failed testing.

Status:Needs work» Needs review

EntityTranslationUITest.php line 40
random failure

#18: aggregator-guzzle-1862524-18.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

I can confirm the typo is fixed ;)

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

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