color_scheme_form_submit() assumes color.inc provides keys that should be optional

So you're basically trying to make base_image optional. That's probably a good idea, but neither a bug, nor major.

Tagging for D8, not sure about backportability, depends on how we fix it.

This is a pretty big WTF when you first start digging into how to make a theme recolorable. I don't care about the issue category but I'd call this a bug since the color module requires you to define/include an unused image file in your theme in order to change a single hex string without throwing errors.

The attached patch checks to see if base_image is set in both places mentioned in the original issue. This allowed me to remove the reference to base.png in my theme's color.inc files and not receive any errors. This should be safe to backport to Drupal 7 as existing themes do not need to be modified.

Tagging as novice.

I'm new to patch testing, but I installed D8 and successfully applied the patch. I want to test the patch, but I don't understand the issue fully.

As a test I renamed base.png and preview.png, cleared cache, modified bartik's colors and saved.

The preview worked fine and the changes also got applied to the theme.

However I did get this error message after saving:

Warning: getimagesize(themes/bartik/color/base.png): failed to open stream: No such file or directory in color_scheme_form_submit() (line 312 of /home/quickstart/websites/drupal8.dev/modules/color/color.module).
Warning: imagecreatefrompng(themes/bartik//color/base.png): failed to open stream: No such file or directory in _color_render_images() (line 506 of /home/quickstart/websites/drupal8.dev/modules/color/color.module).
Warning: imagesx() expects parameter 1 to be resource, boolean given in _color_render_images() (line 507 of /home/quickstart/websites/drupal8.dev/modules/color/color.module).
Warning: imagesy() expects parameter 1 to be resource, boolean given in _color_render_images() (line 508 of /home/quickstart/websites/drupal8.dev/modules/color/color.module).
Warning: imagecreatetruecolor(): Invalid image dimensions in _color_render_images() (line 511 of /home/quickstart/websites/drupal8.dev/modules/color/color.module).
Warning: imagealphablending() expects parameter 1 to be resource, boolean given in _color_render_images() (line 512 of /home/quickstart/websites/drupal8.dev/modules/color/color.module).
Warning: imagecopy() expects parameter 1 to be resource, boolean given in _color_render_images() (line 539 of /home/quickstart/websites/drupal8.dev/modules/color/color.module).
Warning: imagedestroy() expects parameter 1 to be resource, boolean given in _color_render_images() (line 542 of /home/quickstart/websites/drupal8.dev/modules/color/color.module).
Warning: imagedestroy() expects parameter 1 to be resource, boolean given in _color_render_images() (line 574 of /home/quickstart/websites/drupal8.dev/modules/color/color.module).

Please help me review this. Thanks. :)

Edit - it's actually the same error as when there is no patch applied. The base image probably gets called somewhere, still?

I restored the .png files, but instead I commented out 'base_image' => 'color/base.png', in color.inc.

Without patch it threw errors and with patch it didn't :)

Didn't see any difference in my actual theme or the preview, though, without the .png files or without 'base_image' defined. I'll have to look into what it actually does.

Re-rolled patch now that D8 moved everything into /core.

Fresh install of Drupal 7.10 I get this error message prior to applying the latest patch from post #3

Warning: getimagesize(themes/bartik/color/base.png): failed to open stream: No such file or directory in color_scheme_form_submit() (line 312 of /home/staging/drupalcore/modules/color/color.module).
Warning: imagecreatefrompng(themes/bartik//color/base.png): failed to open stream: No such file or directory in _color_render_images() (line 506 of /home/staging/drupalcore/modules/color/color.module).
Warning: imagesx() expects parameter 1 to be resource, boolean given in _color_render_images() (line 507 of /home/staging/drupalcore/modules/color/color.module).
Warning: imagesy() expects parameter 1 to be resource, boolean given in _color_render_images() (line 508 of /home/staging/drupalcore/modules/color/color.module).
Warning: imagecreatetruecolor(): Invalid image dimensions in _color_render_images() (line 511 of /home/staging/drupalcore/modules/color/color.module).
Warning: imagealphablending() expects parameter 1 to be resource, boolean given in _color_render_images() (line 512 of /home/staging/drupalcore/modules/color/color.module).
Warning: imagecopy() expects parameter 1 to be resource, boolean given in _color_render_images() (line 539 of /home/staging/drupalcore/modules/color/color.module).
Warning: imagedestroy() expects parameter 1 to be resource, boolean given in _color_render_images() (line 542 of /home/staging/drupalcore/modules/color/color.module).
Warning: imagedestroy() expects parameter 1 to be resource, boolean given in _color_render_images() (line 574 of /home/staging/drupalcore/modules/color/color.module).

After applying the latest patch:

git apply -v check-if-base-image-isset-924322.patch
Checking patch modules/color/color.module...
Applied patch modules/color/color.module cleanly.

and clearing cache I still get the same errors

Line 312 still gets called.
list($width, $height) = getimagesize($source);

Works as expected against D8. However, using Bartik to test is probably not ideal. Is there another theme that doesn't have base images?

If you use barktik to test, instructions for test case seems to be to remove:
rm core/theme/bartik/color/base.png
rm core/theme/bartik/color/preview.png

However, since bartik comes with images, it also sets 'base_image' => 'color/base.png' (which is why @Jean Gionet encountered the error) Bartik thinks is needs images... and when removing them, then you no longer need (and shouldn't) set 'base_image'

so I commented that out line 131 of core/themes/bartik/color/color.inc to:
//'base_image' => 'color/base.png',

and works as expected.

It probably makes sense to have another test case... so I'm not going to RTBC. But I'm pretty confident this works if there was a clean test case.

as @mike stewart indicated, if you comment out line 131 from the color.inc file from the Bartik theme no errors are displayed. (so the patch does work)

I'm assuming that the Bartik theme would have to be updated at the same time if this patch is to be rolled out.

I've submitted issue #1366594: Remove Color Module Base Image Dependency from Bartik with a patch which will fix Bartik to remove uneeded base_image. The patched version from 1366594 of Bartik can then be used as a unit test for this issue -- plus cleans up Bartik.

To test:

First apply patch from #7 of this issue. Then apply patch from #1366594: Remove Color Module Base Image Dependency from Bartik to use as unit test of #7 above.

This issue has two unit test cases. Themes that have/use base_images, and theme's that don't.

1) The patch in #7 works for the first case: in a theme that has base_image is set -- such as current state of Bartik -- and that's a good test case for themes that use image.

2) This issue is about eliminating the requirement of color-able images (and hence the need for the base_image to be set). But due to this bug, D8 only has themes with color-able images. (Bartik) So I created #1366594: Remove Color Module Base Image Dependency from Bartik

However, the patch I submitted for #1366594 failed. If I understand the error log from the test, it seems it failed due to fact this issue hasn't been committed. So I've created a combo "test patch" (attached) that will allow us to patch both and use for second test case.

So how this should work:
1) Apply patch #7 to a clean Drupal 8.x setup.
2) Then test if you can change colors in Bartik. It should work without manual modification of files. This shows the patch works with Themes that have base_image set.
3) Then revert to a clean Drupal 8.x setup.
4) Apply the attached patch. (a combo of #7 plus the #8 patch to Bartik #1366594)
5) Repeat the test to change colors in Bartik. It should work. The test this time however is against Bartik without base_image set. If it passes it proves the requirement for base_image is removed and test in #2 shows it functions on themes that have it.

Alternatively, do steps 1 & 2 above, then apply patch #8 from #1366594 instead of steps 3 & 4. Step 5 should be same results.

Assuming everything is good. It should be proof the #7 above works as stated and can be RTBC. Which should also allow #1366594: Remove Color Module Base Image Dependency from Bartik to move forward.

after a clean D8 install,
then applying patch #7 of this post
and applying patch #8 from Mike Stewart's Bartik theme post everything is loading error free.

+1 on the rtbc. Its working as expected.

Nice, this is a surprisingly simple patch for such a WTF-y issue. It sounds like this fix is also needed to unblock #1366594: Remove Color Module Base Image Dependency from Bartik, which in turn is needed to add the test coverage described in #13? Is that the case?

• If possible, we need to add some test coverage for this issue along with the patch. Can we test with a theme other than Bartik, for example?
• If there is no way to add test coverage without #1366594: Remove Color Module Base Image Dependency from Bartik going in, I wonder if we shouldn't roll both patches together in a single issue, and add the tests at the same time?

Edited.

CNW for tests.

#13: test-patch-bartik-without-base-image-924322--13.patch queued for re-testing.

Resetting to CNW for tests.

Test is on the way

Ok. I am looking now for other themes to replicate the issue.

I see we're kinda working on the same thing here: http://core.drupalofficehours.org/task/746 and http://core.drupalofficehours.org/task/420 :-).

But in this case, both tests would definitely overlap, which is a waste of time, no ?

You're in the code sprint room :-) ? If so, where are you seated ? I'm in the back, far right (next to Webchick).

This is actually covered by the core test.

If you apply the patch from #1366594: Remove Color Module Base Image Dependency from Bartik to Bartik, you remove the base image. In that case, the tests fail. That's what this issue is all about.

THEN, if you apply the patch from #7, the tests pass, because it removes the necessity to have a base image.

Just to be sure, if you revert the patch to Bartik (which means you still use a base image), and run the tests, they still pass (as they should).

So, as stated in #16, this should be rolled as a single issue / patch (as in #13).

As a side note: how could we include tests that include a base image for recoloring ? Bartik doesn't do it, no other core theme does. Should a new theme be included, just for testing purposes ? Could themes be hidden, just like modules, for testing ?

Unlike @sun (and now the component maintainer for color), I agree with #3. Categorizing this back to a major bug. Also not sure about backport compatibility, but will leave the tag for now.

Rerolled the patch as instructed here https://drupal.org/patch/reroll. I also applied the patch to a clean d8 install and I was able to modify Bartik's colors, but some more experienced testers might need to have a look.

PS: this only my second patch reroll so please tell me if I'm doing something wrong.

Adjusting the scope a bit.
We should be making a color.inc file as simple as possible to author by providing sane defaults and not assuming that optional keys are set.

I added test coverage, and removed the superfluous changes around the memory limit.

Because there are several hunks that are just indentation, providing a git diff -w patch as well.

+++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/color/color.inc
--- /dev/null
+++ b/core/modules/color/tests/modules/color_test/themes/color_test_theme/color/preview.css

Grr, we don't need this, I'll have to remove it. I'll wait for the patches to come back first.

Okay, that passed and failed accordingly. Removed the stray CSS file from my earlier testing. This should be good now.

@tim.plunkett this is awesome! Thanks for taking this on. I've reviewed the patch, code is perfect (naturally :) and solves the original report and then some. Test only patch failed and latest patch is green. Marking RTBC.

This looks like great clean-up. Committed and pushed to 8.x. Thanks!

Moving down to 7.x.