Example from image_style_deliver:
drupal_add_http_header('Status', '500 Internal Server Error');
print t('Error generating image.');
drupal_exit();
If Russion language enabled I get the following message "Ошибка генерации изображения." instead of "Ошибка генерации изображения.". Because the content-type header is not correctly specified.
--
I do not understand why drupal has to send such messages.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | D7-image-8.patch | 1.01 KB | joshi.rohit100 |
| #1 | image.patch | 1.03 KB | droplet |
Comments
Comment #1
droplet commentedMissing the charset
and according to API doc, we always need to include charset in header:
Comment #2
droplet commentedneeds review
Comment #3
kscheirer#1: image.patch queued for re-testing.
Comment #5
claudiu.cristeaIn D8 we are using now
\Symfony\Component\HttpFoundation\Responseto respond and that one is automatically adding the content type header. Moving this issue back to D7 as it has no scope in D8.Comment #6
dcam commented#1 is a patch made for Drupal 8, but the function is the same now in D7 as it was in D8 when the patch was first made. I think the only reason the patch doesn't apply to D7 is because the D8 directory structure was different.
So #1 basically just needs to be rerolled. The two lines will have to be copied manually into the same places in D7.
Comment #7
dcam commentedComment #8
joshi.rohit100Rerolled #1 . Please review now.
Comment #9
dcam commentedIf I knew how to manually trigger these errors for testing, I would. As it is, the code looks alright. I checked core for other instances where we're outputting error Status headers such as in drupal_deliver_html_page(). It does has the same Content-Type header with charset.
Since I don't know how to manually test for this issue, I don't know if there's a way to add an automated test. It seems like we might want one, but since this is only a minor issue then maybe it's not important. I'll go ahead and RTBC it on the basis of the code review. If a committer thinks it needs additional testing/automated tests then it will be bumped back down to Needs Work.
Comment #12
dcam commentedComment #17
dcam commentedComment #20
dcam commentedComment #23
dcam commentedComment #24
David_Rothstein commentedCommitted to 7.x - thanks!
Yeah, I agree, something like this probably doesn't need an automated test.