The system settings page allows the GD toolkit to be selected even if it is not installed. The error is detected when the form is next loaded, rather than when it is submitted. This is a legacy of Drupal 4.6, which did not provide a means to prevent invalid selections from being submitted. In Drupal 4.7 and Drupal 5, the form error prevents any valid settings from being submitted once the GD toolkit option has been submitted.

The attached patch causes validation to be performed before the form is submitted rather than after. It patches both system.module and image.inc. An equivalent patch for Drupal 4.7 is available in issue 102962.

CommentFileSizeAuthor
#35 image_gd_inc.patch5.51 KBaspilicious
#33 102987-validation-for-gd.patch1.79 KBbrianV
#27 image.gd_._102987_6.4.patch1.46 KBBèr Kessels
#15 image.gd_102987.patch1.84 KBdrewish
#10 image.inc_160870.patch1.58 KBdrewish
#3 gd-toolkit_2.patch2.29 KBDarren Oh
#1 gd-toolkit_1.patch2.61 KBDarren Oh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
FileSize
gd-toolkit_1.patch2.61 KB

Missed the patch.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs review » Needs work

patching file modules/system/system.module
Hunk #1 succeeded at 739 with fuzz 2.
patching file includes/image.inc
Hunk #2 FAILED at 190.
1 out of 2 hunks FAILED -- saving rejects to file includes/image.inc.rej

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
Status: Needs work » Needs review
FileSize
gd-toolkit_2.patch2.29 KB

Updated patch for latest code.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
Status: Needs review » Reviewed & tested by the community

It would be nice to get this committed. Since there was an attempt to commit it once, and the only change was to re-diff for the current code, I'm setting this as ready to be committed. All Image toolkit users will thank you for the privilege of using up-to-date code.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Version: 5.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs work

Trying to commit the code does not imply that it was reviewed. It is a way of finding code that isn't ready for a review. So there has not been a second reviewer.

This should be added to Drupal 6 first, and then back-ported.

I think this should work with Drupal's status report page; the validation code for that starts at system_requirements() in system.install.

Otherwise, validation code should live next to the form it validates and FALSE should be capitalized.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

This should have been taken care of a long time ago. I can't spend my time keeping a patch in sync with all the changes in Drupal. If you prefer keeping Drupal 4.6 FormAPI code in HEAD to committing this patch, go ahead.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

Sorry for your difficulty, but I tried to do my best to provide a valuable review. The blocking issues, which are not Drupal-6-specific, were mentioned in the last paragraph of my review.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

I'm sorry, but this has been a problem since Drupal 4.7 was released, and I'm not seeing evidence of a commitment to fix it. It was just recently fixed in the 4.7 branch, but it looks destined to re-appear in future versions. Before I consider beautifying the patch, I want a promise that I won't have to fix this again.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented
Status: Needs work » Closed (won't fix)

I have improved the patch as much as I'm going to. Drumm has rejected the patch because it doesn't fix as much as he wants, so I'm removing this from the issue queue.

drewish’s picture

drewish CreditAttribution: drewish commented
Status: Closed (won't fix) » Needs review
FileSize
image.inc_160870.patch1.58 KB

Darren Oh, marked http://drupal.org/node/160870 as a duplicate of this. i'm posting the patch i'd worked out for that. I'd with drumm's goal of a more comprehensive way of checking the requirements but since it's post freeze. I'm just going to submit the smallest set of changes that will fix the bug.

cornfed’s picture

cornfed CreditAttribution: cornfed commented

Is there a patch for the 5.2 version? I'm running into exactly this problem.

drewish’s picture

drewish CreditAttribution: drewish commented

cornfed, if you're using imagemagick, you might want to try to update to the newest version of the toolkit.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

That won't make any difference. It's the GD toolkit that causes the problem. Since it's still using the Drupal 4.6 Forms API, it reports an error before the form is submitted (disable the GD library extension to see for yourself). The error will prevent you from choosing another toolkit if the GD library is missing.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

By the way, the hook_requirements() remark in #5 is irrelevant, since we're not dealing with requirements or even a module.

drewish’s picture

drewish CreditAttribution: drewish commented
FileSize
image.gd_102987.patch1.84 KB

last patch no longer applied. i think this is a little cleaner since it avoids the duplicate call to image_gd_check_settings().

DrupalTestbedBot tested drewish's patch (http://drupal.org/files/issues/image.gd_102987.patch), the patch passed. For more information visit http://testing.drupal.org/node/109

cornfed’s picture

cornfed CreditAttribution: cornfed commented

so, I'm assuming there isn't a 5.2 patch. this is pretty critical for me, my system administrator is pretty insistent on not installing GD toolkit. Although imagemagick shows up as an option, I am unable to select it. any tips are greatly appreciated!

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

This issue was originally opened to fix Drupal 5. Since the image toolkit code still hasn't changed, the patch in #3 should work for 5.2.

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle commented

can you provide steps how to test this patch

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

You need to add a toolkit to the includes directory. At the moment, I only know of the ImageMagick toolkit at
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/image/image...

You will then have the option to choose your toolkit at admin/settings/image-toolkit. If the page loads without an error, the patch is successful.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

Almost forgot: you must disable the PHP GD extension to test this patch.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Darren Oh - the patch applies cleanly, and I pulled the toolkit down into my includes directory - was able to select between each one - however I have GD installed - do I have to disable it to test properly?

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

Yes, the purpose of the patch is to allow other toolkits to be selected when the GD toolkit is not available. Currently the GD toolkit validation error prevents this.

boydkelly’s picture

boydkelly CreditAttribution: boydkelly commented

Hi,

I am using Drupal 5.5, and had to switched to a CentoOS 5.0 box that does not have php compiled with GD. I have exactly the problem on this thread. Can the above patch work for me? How do I apply it?

Thanks,

bk

alanburke’s picture

alanburke CreditAttribution: alanburke commented

subscribe

Bèr Kessels’s picture

Bèr Kessels CreditAttribution: Bèr Kessels commented

#210469 was marked a duplicate of this issue. It contains some code (for reference?)

Bèr Kessels’s picture

Bèr Kessels CreditAttribution: Bèr Kessels commented
Status: Needs review » Reviewed & tested by the community
FileSize
image.gd_._102987_6.4.patch1.46 KB

Rerolled patch against Drupal 6.4 and attached.
Works as advertised. The change in logic is minimal and solves the issue in the IMO correct way.

drewish’s picture

drewish CreditAttribution: drewish commented
Version: 6.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

this never made it into HEAD so i'm bumping the version and kicking the status down.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh commented

This is being treated like a feature request. Image handling is likely to be removed from core before this gets fixed.

Bèr Kessels’s picture

Bèr Kessels CreditAttribution: Bèr Kessels commented

It is a bug on 6.x. But not yet on 7, since the imagehandling will be changed and/or removed there afaik.

It is by no means a feature request. One could argue that it is some kind of "wrongly implemented feature that needs improvement", but in my world that counts as a bug too :_)

And also, if this patch gets into 7, then it *still* needs to be applied to 6 as well. (and possibly to 5, but that I cannot confirm).

So, is it not more valid to fix this in 6 and from there see how/if/when to apply this to future releases?

drewish’s picture

drewish CreditAttribution: drewish commented

Come on Ber, you've been around drupal long enough to know that a bug that's made it from D5 to now isn't going to be magically fixed by a mythical rewrite ;) we need to fix it in Head and then backport.

NukeHavoc’s picture

NukeHavoc CreditAttribution: NukeHavoc commented

+1.

The patch works fine for Drupal 5.14; I haven't tried the Drupal 6 version. It'd be nice to get this bug fix into both 5 and 6, seeing as how it prevents the use of any image kit other than GD. Usually it's not a big deal, as most places do support GD, but it was a headache when I was trying to get Drupal working with ImageMagick working on my Mac's test environment.

brianV’s picture

brianV CreditAttribution: brianV commented
Status: Needs work » Needs review
FileSize
102987-validation-for-gd.patch1.79 KB

D7 patch!

Status: Needs review » Needs work

The last submitted patch failed testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Needs work
FileSize
image_gd_inc.patch5.51 KB

I rerolled...
This patch isn't rdy yet it needs more documentation but I don't have the knowledge to write it :).

I also fixed a dozen doc style problems in the file.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review
Issue tags: +Documentation
retester2010’s picture

retester2010 CreditAttribution: retester2010 commented
Status: Needs work » Needs review
Issue tags: -Documentation

#35: image_gd_inc.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, image_gd_inc.patch, failed testing.