The idea behind the image.gd.inc file is that it should be usable for people who are using GD 1 and GD 2.
Unfortunatly, the way it is written now, only support for GD 2 can be given because:

Current way of determining GD 2, is not right:

if (function_exists('imageCreateTrueColor')) {
  // GD 2 Handling;
}
else {
  // GD 1 Handling
}

this doesn't work.. imageCreateTrueColor() was already implemented in GD 1, only it failed to work properly..
See these pages:
http://www.php.net/imagecratetruecolor#25234 &
http://www.php.net/imagecratetruecolor#25487

According to this comments, we should implement this like this:

// Silence errors using the @
$image = @imageCreateTrueColor(......);

if (!$image) {
  // GD 1 Handling;
}
CommentFileSizeAuthor
#16 image-inc_2.patch6.3 KBwalkah
#15 image-inc_1.patch5.39 KBwalkah
#10 image.gd1.inc2.07 KBStefan Nagtegaal
#9 image-inc_0.patch2.99 KBStefan Nagtegaal
#1 image-inc.patch2.26 KBStefan Nagtegaal
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Stefan Nagtegaal’s picture

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented
Assigned: Unassigned » Stefan Nagtegaal
FileSize
image-inc.patch2.26 KB

Attached patch improves:
- the way which deals with the detection of GD 1 or GD 2;
- error messages as reported by Steven here: http://drupal.org/node/17645;

After applying this to HEAD/Drupal 4.6 RC, we can close the following issues:
- http://drupal.org/node/17645;
- http://drupal.org/node/13027;

Please test/comment and apply attached patch..

Dries’s picture

Dries CreditAttribution: Dries commented

Isn't the approach taking in http://drupal.org/node/13027 nicer? It doesn't surpress warnings/errors. Maybe that approach doesn't work?

Bèr Kessels’s picture

Bèr Kessels CreditAttribution: Bèr Kessels commented

-1 on supressing errors with @. Its ugly, and hardly used in Drupal. We should use our own error handling at all times.

Stefan Nagtegaal’s picture

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented

I will make a new patch, with the more elegant approach you mentioned Dries...

Stefan Nagtegaal’s picture

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented

As noticed before in this issue.. The function imageCreateTrueColor _does_ exist, even in versions of GD before 1.8 only it was broken..
So, function_exist() doesn't work on that the function, because it really _does_ exist, but the fact is that it's simply broken..

So unfortunatly, the more elegant solution doesn't seem to work..

pz’s picture

pz CreditAttribution: pz commented

Stupid question, why not just use the gd_info function?

Dries’s picture

Dries CreditAttribution: Dries commented

What versions of PHP have those old GD toolkits?

PHP 4.3.10 comes with GD 2.0.28, it seems, as does PHP 5.0.3.

PHP 4.3.3, Drupal's current minimum PHP requirement, comes with GD 2.0.15.

From the looks, we don't need GD 1 handling at all ... (but we might need a global version check).

Thoughts?

Stefan Nagtegaal’s picture

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented

PHP 4.1.2 comes with GD 1.62..

The nasty/hackish code with the build-in GD toolkit, is exactly why I proposed to split the image.inc into a separate image.gd1.inc and image.gd2.inc.

I would propose a patch which does:
- remove all the GD < 2 function calls like imageCreate() and imageCopyResized() and have a real GD 2 compatible image.inc;
- introduce a new image.gd1.inc file into Walkahs sandbox; If the module is being moved to the contributions, the image.gd1.inc is also available for people without proper GD 2 support;
- more userfriendly and helpfull error reporting for the image-api..

If we agree about this, I'll make patches for inclusion into 4.6.. Which does contain the before mentioned problems with the current usage..

Please, I would like to have some feedback about this...

Stefan Nagtegaal’s picture

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented
FileSize
image-inc_0.patch2.99 KB

As proposed above, patch for image.inc..

Please comment or apply...

The image.gd1.inc file wil be posted after this post...

Stefan Nagtegaal’s picture

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented
FileSize
image.gd1.inc2.07 KB

And the image.gd1.inc file for Walkahs sandbox...

Tell me what you all think about this..

Dries’s picture

Dries CreditAttribution: Dries commented

I'm OK with this! I'd like to await James' input though.

Stefan Nagtegaal’s picture

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented

Walkah, are you out there???
Ber, do you agree with Dries that this is a better approach?

Bèr Kessels’s picture

Bèr Kessels CreditAttribution: Bèr Kessels commented

I like this approach because it: uses a good default, and uses that well. Provides flexibility for those not wanting that default, without extra complexity. Uses modules to add extra functionality instead of configuration options.
big +1 from me.

walkah’s picture

walkah CreditAttribution: walkah commented

Hey guys, sorry I've been silentlly watching this thread. Here's my take:

+1 - I like the approach, and I agree that (especially now that we're requiring 4.3.x) making the "default" support gd2 only isn't a bad idea.

I'm working on some additions to this patch, however, to basically make the toolkit checking a bit more robust (working on top of Stefan's). Great job Stefan, keep it up!

walkah’s picture

walkah CreditAttribution: walkah commented
FileSize
image-inc_1.patch5.39 KB

OK - attached is said patch. It also includes a fix to the recent system.module patch (where it was reporting that no toolkits were installed if only the built-in one was).

I've also included a link on admin/settings to http://drupal.org/project/image where I will post information about getting other toolkits, etc. (I think that makes sense).

comments?

walkah’s picture

walkah CreditAttribution: walkah commented
FileSize
image-inc_2.patch6.3 KB

cleaner version. busted by the punctuation police. also fixes the settings page (merged that too quickly).

Dries’s picture

Dries CreditAttribution: Dries commented

Committed to HEAD.

Anonymous’s picture

(not verified) CreditAttribution: commented
seanr’s picture

seanr
he/him
CreditAttribution: seanr commented
Version: » 4.7.x-dev
Status: Closed (fixed) » Active

This still does not work in 4.7. Also, if GD2 isn't installed, why doesn't it fall back on imageCreate() and imageCopyResized() instead of just crapping out? Seems like that should be easy enough to do.

Stefan Nagtegaal’s picture

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented
Status: Active » Closed (fixed)

please do not re-open closed bgu reports..