Posted by Darren Oh on December 13, 2006 at 1:32am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | system.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | documentation |
Issue Summary
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.
Comments
#1
Missed the patch.
#2
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
#3
Updated patch for latest code.
#4
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.
#5
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.
#6
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.
#7
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.
#8
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.
#9
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.
#10
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.
#11
Is there a patch for the 5.2 version? I'm running into exactly this problem.
#12
cornfed, if you're using imagemagick, you might want to try to update to the newest version of the toolkit.
#13
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.
#14
By the way, the hook_requirements() remark in #5 is irrelevant, since we're not dealing with requirements or even a module.
#15
last patch no longer applied. i think this is a little cleaner since it avoids the duplicate call to image_gd_check_settings().
#16
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
#17
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!
#18
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.
#19
can you provide steps how to test this patch
#20
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.
#21
Almost forgot: you must disable the PHP GD extension to test this patch.
#22
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?
#23
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.
#24
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
#25
subscribe
#26
#210469 was marked a duplicate of this issue. It contains some code (for reference?)
#27
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.
#28
this never made it into HEAD so i'm bumping the version and kicking the status down.
#29
This is being treated like a feature request. Image handling is likely to be removed from core before this gets fixed.
#30
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?
#31
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.
#32
+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.
#33
D7 patch!
#34
The last submitted patch failed testing.
#35
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.
#36
#37
#35: image_gd_inc.patch queued for re-testing.
#38
The last submitted patch, image_gd_inc.patch, failed testing.