image_style_deliver uses module_invoke_all to call hook_file_download, and module_invoke_all use array_merge_recursive to merge the results. This will be a problem if there is more than one hook_file_download returning the same header.
Example.
If two modules provide the header 'Content-Type' => 'image/jpeg', image_style_deliver will get an headers array looking like this.

array(
'Content-Type' => array(
'image/jpeg',
'image/jpeg',
),
)

And the result from that is that drupal_add_http_header will throw a notice (array to string).
I think image_style_deliver should behave like file_download does. So I created a patch to fix this... *phu* I hope anyone can read and understand this ;)

CommentFileSizeAuthor
#64 image_style_deliver_can-1891228-64.patch2.24 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,765 pass(es). View
#62 image_style_deliver_can-1891228-62.patch2.51 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,764 pass(es). View
#60 image_style_deliver_can-d8-1891228-60-test-only.patch1.95 KBeiriksm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_style_deliver_can-d8-1891228-60-test-only.patch. Unable to apply patch. See the log in the details link for more information. View
#57 image_style_deliver_can-1891228-50.patch2.63 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,760 pass(es). View
#54 image_style_deliver_can-1891228-47-test-only.patch1.73 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 41,764 pass(es). View
#50 image_style_deliver_can-1891228-50.patch2.63 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,602 pass(es). View
#48 image_style_deliver_can-1891228-21_0.patch2.57 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,596 pass(es). View
#47 image_style_deliver_can-1891228-47-test-only.patch1.73 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,602 pass(es). View
#44 image_style_deliver_can-1891228-44.patch2.31 KBeiriksm
FAILED: [[SimpleTest]]: [MySQL] 41,597 pass(es), 0 fail(s), and 2 exception(s). View
#42 image_style_deliver_can-1891228-42.patch2.29 KBeiriksm
FAILED: [[SimpleTest]]: [MySQL] 41,570 pass(es), 53 fail(s), and 33 exception(s). View
#41 image_style_deliver_can-1891228-41-test-only.patch552 byteseiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,600 pass(es). View
#40 image_style_deliver_can-1891228-40-test-only.patch2.16 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,592 pass(es). View
#39 image_style_deliver_can-1891228-39-test-only.patch991 byteseiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,597 pass(es). View
#38 image_style_deliver_can-1891228-38-test-only.patch2.04 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,607 pass(es). View
#37 image_style_deliver_can-1891228-37-test-only.patch1.92 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,609 pass(es). View
#34 image_style_deliver_can-1891228-34-test-only.patch1.8 KBeiriksm
FAILED: [[SimpleTest]]: [MySQL] 41,600 pass(es), 8 fail(s), and 10 exception(s). View
#33 image_style_deliver_can-1891228-33.patch1.96 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,605 pass(es). View
#30 image_style_deliver_can-1891228-30-test-only.patch1.65 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,605 pass(es). View
#24 image_style_deliver_can-1891228-21-test-only.patch1.65 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,503 pass(es). View
#24 image_style_deliver_can-1891228-21.patch2.57 KBeiriksm
PASSED: [[SimpleTest]]: [MySQL] 41,501 pass(es). View
#21 image_style_deliver_can-1891228-21-test-only.patch1.71 KBeiriksm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_style_deliver_can-1891228-21-test-only.patch. Unable to apply patch. See the log in the details link for more information. View
#21 image_style_deliver_can-1891228-21.patch2.65 KBeiriksm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_style_deliver_can-1891228-21.patch. Unable to apply patch. See the log in the details link for more information. View
#16 image_system-can_create_invalid_headers-1891228-16.patch938 bytesStefanPr
PASSED: [[SimpleTest]]: [MySQL] 41,516 pass(es). View
#6 image_style_deliver_shouldnt_use_module_invoke_all-6.patch938 byteslogaritmisk
PASSED: [[SimpleTest]]: [MySQL] 40,881 pass(es). View
image_style_deliver_shouldnt_use_module_invoke_all-0.patch942 byteslogaritmisk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_style_deliver_shouldnt_use_module_invoke_all-0.patch. Unable to apply patch. See the log in the details link for more information. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

logaritmisk’s picture

Status: Active » Needs review

Changed status, there is a patch to test.

mariancalinro’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

It works for me.

almc’s picture

The patch didn't apply automatically for Drupal 7.26 so I'll try to trigger re-test here, however after I applied it manually in the core image.module, the issue with 'array to string conversion' notices I had with File Entity is gone now. Thank you for the patch.

almc’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, image_style_deliver_shouldnt_use_module_invoke_all-0.patch, failed testing.

logaritmisk’s picture

Status: Needs work » Needs review
FileSize
938 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,881 pass(es). View

Re-roll patch against dev

mariancalinro’s picture

Status: Needs review » Reviewed & tested by the community

I will rtbc again, I have this live on some sites, and it's fixing the issue.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs backport to D7

This would need to go into Drupal 8 first.

It could also use a code comment explaining why it's done this way, and I'm wondering if there are other places in the codebase that invoke hook_file_download() that have the same problem?

thtas’s picture

The error that brought me here was:

Notice: Array to string conversion in drupal_send_headers() (line 1221 of /var/www/drupal-7.27/includes/bootstrap.inc).

For future googlers.

Patch #6 solved the problem

joelpittet’s picture

Issue tags: +Needs tests

D7:

TLDR; this is an over explanation of the fact that I should have read the issue summary, it would have saved me time.

I had a private file being rendered as a download in a custom entity.
following hooks were called:

file_entity_file_download
image_file_download

Both returned the same array.

$result = [
	'Content-Type' => "image/jpeg",
	'Content-Length' => "15140",
	'Cache-Control ' => "private",
];

Within module_invoke_all($hook) we are doing a array_merge_recursive

$return = [
	'Content-Type' => "image/jpeg",
	'Content-Length' => "15140",
	'Cache-Control ' => "private",
];
$result = [
	'Content-Type' => "image/jpeg",
	'Content-Length' => "15140",
	'Cache-Control ' => "private",
];

$result = array_merge_recursive($result, $return);
var_dump($result);

Results:

array(3) {
  'Content-Type' =>
  array(2) {
    [0] =>
    string(10) "image/jpeg"
    [1] =>
    string(10) "image/jpeg"
  }
  'Content-Length' =>
  array(2) {
    [0] =>
    string(5) "15140"
    [1] =>
    string(5) "15140"
  }
  'Cache-Control ' =>
  array(2) {
    [0] =>
    string(7) "private"
    [1] =>
    string(7) "private"
  }
}

Ok all that being said, maybe a test is needed to show the problem.

seanenroute’s picture

Hi, this error is still happening in 7.8. Will the patch be added to core fairly soon, I'm not keen on patching a core module.

kyuubi’s picture

Hi,

I am having the same issue.

Patch fixed it for me.

When can we expect to see this commited?

Thanks,

kyuubi’s picture

I just noticed this was introduced with the latest core update to 7.31.

I can see from the doing a diff that the code this patch adds was in core but got removed?

berte’s picture

I just noticed this was introduced with the latest core update to 7.31.

Also present in core 7.34.

Patch in #6 worked for me.

pslcbs’s picture

Same issue on 7.35.
Applied #6 and works great too.

Thanks!

StefanPr’s picture

Version: 8.0.x-dev » 7.36
Issue summary: View changes
FileSize
938 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,516 pass(es). View

I've added a patch for D7.36

Johnny vd Laar’s picture

Issue summary: View changes
mpotter’s picture

Status: Needs work » Needs review

Marking this Needs Review to get #16 tested in latest D7. But then I know somebody will just change it to 8.0.x again. But we need this patch in Open Atrium to fix the send_header errors we are seeing.

marcvangend’s picture

Version: 7.36 » 7.x-dev

If we want it fixed in D7, it should at least be fixed in 7.x-dev.

eiriksm’s picture

FileSize
2.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_style_deliver_can-1891228-21.patch. Unable to apply patch. See the log in the details link for more information. View
1.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_style_deliver_can-1891228-21-test-only.patch. Unable to apply patch. See the log in the details link for more information. View

OK, let's get this in. I really need this for a handful of sites.

Probably needs tests. So here is a test-only patch and patch with added test.

The last submitted patch, 21: image_style_deliver_can-1891228-21.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: image_style_deliver_can-1891228-21-test-only.patch, failed testing.

eiriksm’s picture

Status: Needs work » Needs review
FileSize
2.57 KB
PASSED: [[SimpleTest]]: [MySQL] 41,501 pass(es). View
1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 41,503 pass(es). View

Oops. Made this in a repo where drupal was not root folder :)

eiriksm’s picture

Hm, I guess the testbot does not consider notices as errors in tests. Either that or it does not consider "Array to string conversion" as a notice (should be added in php 5.4, right?)

For the record, I checked if this bug exists in d8, it does not.

Not sure how to adjust the test to force it to fail on the testbot, through. image_style_deliver calls drupal_exit, so that does not work. Anyway, leaving as needs review for now.

andrew@oscailte.org’s picture

This patch works for me in 7.38

eiriksm’s picture

If it works and you think the patch code looks ok, feel free to set the status to "Reviewed & tested by the community" to try to get this commited. :)

mariancalinro’s picture

About the tests, I did a little digging around, and the only idea I found is this.

Maybe that would work for making tests fail, even if it's ugly.

eiriksm’s picture

FileSize
1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 41,605 pass(es). View

Hm, really strange that this test-only patch is not failing. Tested it on different platforms too. Trying once more...

mariancalinro’s picture

The test will not fail because notices do not trigger fails in tests. For a failing test, you need to intercept the notice, and trigger a fail. To intercept the notice, you can use a custom error handler, and in there you do a string match to see if the error message is "Array to string conversion". If the string matches, you trigger a fail, otherwise at the end of the test you trigger a pass.

You can find a link to an example in #29.

eiriksm’s picture

Status: Needs review » Needs work

On my computer, across 3 versions of php and in both GUI and CLI versions of running the tests, and across 2 platforms (both on my Mac and in a virtual ubuntu machine) they are always being marked as failing, when there is a notice. According to someone on IRC, this should also be the case for the testbots.

I'll see if I can make it fail in another way though. And worst case, look at that link. Seems a little elaborate, though :)

eiriksm’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
PASSED: [[SimpleTest]]: [MySQL] 41,605 pass(es). View

How about this, then...

eiriksm’s picture

FileSize
1.8 KB
FAILED: [[SimpleTest]]: [MySQL] 41,600 pass(es), 8 fail(s), and 10 exception(s). View

Hm, maybe try to log in the user to make sure it is not a caching issue. And also assert server response. This is so strange...

Status: Needs review » Needs work

The last submitted patch, 34: image_style_deliver_can-1891228-34-test-only.patch, failed testing.

mariancalinro’s picture

The tests in #34 failed because the login failed, $this->admin_user is not existing.

What if we throw an exception in drupal_add_http_header() if the $name is not a string, as the function assumes that the $name argument is a string? That would make the tests a lot easier to write, and would make possibly bad code in contrib easier to identify.

eiriksm’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
PASSED: [[SimpleTest]]: [MySQL] 41,609 pass(es). View

Yeah, i saw that, went a bit fast there. :)

So, I finally got this to not fail for me too. And throwing an exception would not fix it, because the test just never triggered the notice (hence it was not passing an array as a header value). Hopefully this will get the test failing.

eiriksm’s picture

FileSize
2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 41,607 pass(es). View

Taking another stab in the dark here. Maybe the test image is getting cached somehow, since it re-uses the same file as another test.

eiriksm’s picture

FileSize
991 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,597 pass(es). View

OK, let's try something else. This will not be the actual test diff, of course, but this makes the test also fail for me. Let's hope the same is the case for the testbot

eiriksm’s picture

FileSize
2.16 KB
PASSED: [[SimpleTest]]: [MySQL] 41,592 pass(es). View

OK, so this is literally obligated to fail :)

eiriksm’s picture

FileSize
552 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,600 pass(es). View

Seriously? Just uploading this in pure spite, to test the testbot.

eiriksm’s picture

FileSize
2.29 KB
FAILED: [[SimpleTest]]: [MySQL] 41,570 pass(es), 53 fail(s), and 33 exception(s). View

OK, then I guess all my effort has always been wasted :). The testbot does not recognise a notice at all, so I guess we have to go with an exception? This one is not really a test-only patch, and I am a bit uncertain if it is a good idea to add this (one could consider this a breaking change, in some weird way).

Status: Needs review » Needs work

The last submitted patch, 42: image_style_deliver_can-1891228-42.patch, failed testing.

eiriksm’s picture

FileSize
2.31 KB
FAILED: [[SimpleTest]]: [MySQL] 41,597 pass(es), 0 fail(s), and 2 exception(s). View

OK then, we allow numbers as well, I guess.

eiriksm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 44: image_style_deliver_can-1891228-44.patch, failed testing.

eiriksm’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
PASSED: [[SimpleTest]]: [MySQL] 41,602 pass(es). View

Ah, so I finally am pretty sure I have figured it out.

On php 5.3, the notice will not be generated, but the string will just become "Array". I was convinced these tests run on 5.4 (since that is what it says on the testbot project page), but I assume it has to be 5.3.

Attached is a patch that fails on both 5.3 and above. So we can finally go back to the simple solution and not throw an exception, probably. Less disruptive this way.

eiriksm’s picture

FileSize
2.57 KB
PASSED: [[SimpleTest]]: [MySQL] 41,596 pass(es). View

Well, thats a lot of back and forth.

Anyway, to summarize:

Last comment, when I thought I had made a test that would fail on php 5.3, I only switched CLI php version, so the web server was still running 5.4. So that was why it was failing locally for me.

So it seems it is impossible to get this to fail on php 5.3. So I'll just reupload the patch from #21. This test would have failed if the test was running on 5.4 >=.

image_style_deliver can end up calling drupal_add_header with an array, but then a couple of lines later, file_transfer also adds the content-type header, thus overriding the array with a string again. So no way to cleanly test this as far as I can see.

Anyway, this patch still fixes the issue (which appearently only is an issue on 5.4 >=), so please test and review.

Sam152’s picture

+++ b/modules/image/image.module
@@ -833,7 +833,19 @@ function image_style_deliver($style, $scheme) {
+          // Throw away the headers received so far.

This comment should explain why the headers are discarded not that they ARE discarded.

+++ b/modules/image/image.module
@@ -833,7 +833,19 @@ function image_style_deliver($style, $scheme) {
+          $headers = array();
...
       if (in_array(-1, $headers) || empty($headers)) {
         return MENU_ACCESS_DENIED;

Does discarding the headers for a -1 result break the access denied later on? -1 will never appear in the array and thus MENU_ACCESS_DENIED will never be returned?

eiriksm’s picture

FileSize
2.63 KB
PASSED: [[SimpleTest]]: [MySQL] 41,602 pass(es). View

This comment should explain why the headers are discarded not that they ARE discarded.

This is a copy-paste from the file_dowload function, which has the exact same code. We throw away the headers, because someone has denied access, and thus no headers should be sent. It does not directly relate to throwing away headers to avoid duplicate headers.

Does discarding the headers for a -1 result break the access denied later on? -1 will never appear in the array and thus MENU_ACCESS_DENIED will never be returned?

No. When a -1 is encountered, the headers are "emptied" (as explained above), the loop is "stopped", and the next if will check for an empty array of headers, which will in turn return MENU_ACCESS_DENIED. Actually I think we can remove the "if (in_array(-1, $headers)" part, because headers will always be empty when there is a -1 result. I'll just try that here, to see if all tests still pass.

mstrelan’s picture

Can we RTBC this?

joelpittet’s picture

Issue tags: -Needs tests

@mstrelan if you've reviewed and tested it, then sure you can.

joelpittet’s picture

The tests only patch in #47 really should be failing if it's testing the problem we are solving and this is really a bug in D7. So i'd keep it say this needs-work for that.

I'm re-posting those so we can get the new CI testbot to try it out with different versions of PHP.

joelpittet’s picture

FileSize
1.73 KB
PASSED: [[SimpleTest]]: [MySQL] 41,764 pass(es). View
eiriksm’s picture

This is not failing because the test is run on the old CI with php 5.3 where this does not generate a notice. Do you know when we can start to test d7 on the new testbot? Then this would fail easily and prove the bug.

Mixologic’s picture

I fired up two ad-hoc tests for this issue to test #47 and #50.

The tests only results are (will be) here:
https://dispatcher.drupalci.org/job/DCI_XML/161/

And the patch is here:
https://dispatcher.drupalci.org/job/DCI_XML/162/

stefan.r’s picture

FileSize
2.63 KB
PASSED: [[SimpleTest]]: [MySQL] 41,760 pass(es). View

In #56 the test-only patch is red with:

Error Message

exception: [Notice] Line 1236 of includes/bootstrap.inc:
Array to string conversion

and #50 fixes it, so re-uploading that.

Anyone else care to give this a final review?

PatchRanger’s picture

Status: Needs review » Reviewed & tested by the community

Also had this problem, #57 fixed it.
Drupal 7.41, PHP 5.6.14.

David_Rothstein’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Needs work

See my comment in #8... I think all of that still applies?

And related to that, for Drupal 7 couldn't we just call file_download_headers() here rather than duplicating its code? (Drupal 8 doesn't have that function, although probably it should be added as part of this issue so it can be reused in the Drupal 8 patch also.)

eiriksm’s picture

Status: Needs work » Needs review
FileSize
1.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image_style_deliver_can-d8-1891228-60-test-only.patch. Unable to apply patch. See the log in the details link for more information. View

As stated in #25 I have tested this on D8 and the bug does not exist there. Granted, this might as well be stated in the IS, it is not so easy to just find randomly in one of the comments.

To be 100% sure, I just tested it again today. But don't take my word for it, here is a test-only patch that should pass on D8. :)

Regarding the other points in #8 and #59:
- We could absolutely comment it better. And use file_download_headers. Definitely. I'll write a new patch with that. (after this d8 patch).
- There is only one other call to module_invoke_all('file_download') in the codebase, and it is also in image.module. But the problem would not apply here, as it does not try to add these headers afterwards. The call is here: https://api.drupal.org/api/drupal/modules!image!image.module/function/im...

stefan.r’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Needs review » Needs work

Back to 7.x

eiriksm’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
PASSED: [[SimpleTest]]: [MySQL] 41,764 pass(es). View

For d7, with more comments and using file_download_headers

Did not do an interdiff since the patch is so small (and the change so apparent) :)

David_Rothstein’s picture

Ah, it looks like Drupal 8 never uses array_merge_recursive() when invoking any hook, that's why.

-      $headers = module_invoke_all('file_download', $image_uri);
-      if (in_array(-1, $headers) || empty($headers)) {
+      // We do not call module_invoke_all('file_download') here because that
+      // calls array_merge_recursive which in turn can make some headers end up
+      // as an array if more than one module returns the same header.
+      // @see https://www.drupal.org/node/1891228
+      $headers = file_download_headers($image_uri);

So since we're calling file_download_headers() I don't think we need this code comment - the reasoning is already documented inside that function and we don't need to explain why we're using the correct API :) (Sorry for the confusion - in my comment from long ago I was probably thinking we needed a comment here because file_download_headers didn't even exist then so calling it wasn't an option.)

There is only one other call to module_invoke_all('file_download') in the codebase, and it is also in image.module. But the problem would not apply here, as it does not try to add these headers afterwards.

Agreed. That one should probably be using file_download_headers() anyway, but isn't needed to fix a bug so doesn't need to be part of this issue.

eiriksm’s picture

FileSize
2.24 KB
PASSED: [[SimpleTest]]: [MySQL] 41,765 pass(es). View

I agree. So here is one with no comments :)

eiriksm’s picture

Issue tags: +dcoslo15
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Yup, that looks good to me, and the patch works as expected.

The last submitted patch, 60: image_style_deliver_can-d8-1891228-60-test-only.patch, failed testing.

doitDave’s picture

Status: Reviewed & tested by the community » Active

*Bump*

I would _really_ like to see this issue not becoming legacy with Drupal 7. Can we have the patch in the very next core release pretty soon, please? Really annoying thing if you work with a lot of private file image styles.

Can I assist in accelerating the solution? Please let me know. Thanks!

doitDave’s picture

Status: Active » Reviewed & tested by the community

accidentally changed issue state (re-rolling)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -needs backport to D7

Yup, the plan was always to get it into the next release - I just left it for a while in case there were additional reviews.

Committed to 7.x - thanks!

Removing backport tag since it turned out this did not need a Drupal 8 fix. If someone wants to forward-port the tests to Drupal 8 I guess it would be nice to have a followup issue for that - it could be created and then linked back here.

  • David_Rothstein committed 03cbe65 on 7.x
    Issue #1891228 by eiriksm, logaritmisk, joelpittet, stefan.r, StefanPr,...
joelpittet’s picture

Thanks @David_Rothstein. I've created a stub issue for those D8 forward port tests #2660564: Forward port tests for image_style_deliver can create invalid headers

doitDave’s picture

Great, thanks from here, too!

Status: Fixed » Closed (fixed)

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