Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#18 | aggregator-guzzle-1862524-18.patch | 1.87 KB | ParisLiakos |
#13 | aggregator-guzzle-1862524-13.patch | 1.87 KB | Berdir |
#12 | aggregator-guzzle-1862524-12.patch | 2.17 KB | Sutharsan |
#6 | aggregator-guzzle-1862524-6.patch | 2.16 KB | Berdir |
#1 | aggregator-guzzle-1862524-1.patch | 2.32 KB | Berdir |
Comments
Comment #1
BerdirConversion 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.
Comment #2
BerdirI 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.
Comment #4
mtdowling CreditAttribution: mtdowling commentedGuzzle 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?
Comment #5
BerdirYes. 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.
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.
Comment #6
BerdirAnother one simplified a bit.
Comment #7
rbayliss CreditAttribution: rbayliss commentedThis 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.
Comment #8
BerdirYes, 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?
Comment #9
rbayliss CreditAttribution: rbayliss commentedHa. Funny timing. I just posted a possible solution here: #1875792: Standardize Guzzle exception handling
Comment #10
YesCT CreditAttribution: YesCT commented#6: aggregator-guzzle-1862524-6.patch queued for re-testing.
Comment #12
Sutharsan CreditAttribution: Sutharsan commentedPatch rerolled.
Comment #13
BerdirRe-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.
Comment #15
Berdir#13: aggregator-guzzle-1862524-13.patch queued for re-testing.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commented+1 for the return addition..no need to trigger _aggregator_parse_opml() if there are no data
rtbc'ing and i hope bot agrees:)
Comment #17
tstoecklerTypo: Faled -> Failed
Applies to the other watchdog()s and dsm()s as well.
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedoops:)
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedEntityTranslationUITest.php line 40
random failure
#18: aggregator-guzzle-1862524-18.patch queued for re-testing.
Comment #21
BerdirI can confirm the typo is fixed ;)
Comment #22
catchCommitted/pushed to 8.x, thanks!