Issue status update for http://drupal.org/node/19611 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Junyor Reported by: Junyor Updated by: Junyor Status: patch Attachment: http://drupal.org/files/issues/image.gd1_0.inc (1.8 KB) Junyor Previous comments: ------------------------------------------------------------------------ March 28, 2005 - 22:46 : Junyor Attachment: http://drupal.org/files/issues/system-image.patch (6.8 KB) The new image handling tools in core have a lot of GD2-specific logic, making image.inc overly cluttered and complicated. If gd2 were just treated like another toolkit (though the only one included by default), then the image.inc code would be much cleaner and more easily maintained. This patch removes all the GD2 stuff from image.inc and puts it into image.gd2.inc. The _settings hook is now doing what it sounds like it should, return settings for the toolkit. I've added a _check hook which will tell if the PHP functions needed are available. If _check fails, then the toolkit will not be listed on admin > settings and an error will be logged in watchdog. I've also update image.gd1.inc, image.imlib.inc, and image.imagick.inc with the same logic. ------------------------------------------------------------------------ March 28, 2005 - 23:08 : Junyor Attachment: http://drupal.org/files/issues/image.gd2.inc (3.07 KB)

Issue status update for http://drupal.org/node/19611 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Junyor Reported by: Junyor Updated by: Junyor Status: patch Attachment: http://drupal.org/files/issues/image.imlib.inc (2.07 KB) Junyor Previous comments: ------------------------------------------------------------------------ March 28, 2005 - 22:46 : Junyor Attachment: http://drupal.org/files/issues/system-image.patch (6.8 KB) The new image handling tools in core have a lot of GD2-specific logic, making image.inc overly cluttered and complicated. If gd2 were just treated like another toolkit (though the only one included by default), then the image.inc code would be much cleaner and more easily maintained. This patch removes all the GD2 stuff from image.inc and puts it into image.gd2.inc. The _settings hook is now doing what it sounds like it should, return settings for the toolkit. I've added a _check hook which will tell if the PHP functions needed are available. If _check fails, then the toolkit will not be listed on admin > settings and an error will be logged in watchdog. I've also update image.gd1.inc, image.imlib.inc, and image.imagick.inc with the same logic. ------------------------------------------------------------------------ March 28, 2005 - 23:08 : Junyor Attachment: http://drupal.org/files/issues/image.gd2.inc (3.07 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:09 : Junyor Attachment: http://drupal.org/files/issues/image.gd1_0.inc (1.8 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:09 : Junyor Attachment: http://drupal.org/files/issues/image.imagick.inc (2.19 KB)

Issue status update for http://drupal.org/node/19611 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Junyor Reported by: Junyor Updated by: Junyor Status: patch @Gordon: They already are separate toolkits. There's *no* GD 1 processing in core at all. This patch separates all the GD 2 specific functions from the general toolkit handling functions. The GD 1 toolkit attached here was provided by Stefan. Junyor Previous comments: ------------------------------------------------------------------------ March 28, 2005 - 22:46 : Junyor Attachment: http://drupal.org/files/issues/system-image.patch (6.8 KB) The new image handling tools in core have a lot of GD2-specific logic, making image.inc overly cluttered and complicated. If gd2 were just treated like another toolkit (though the only one included by default), then the image.inc code would be much cleaner and more easily maintained. This patch removes all the GD2 stuff from image.inc and puts it into image.gd2.inc. The _settings hook is now doing what it sounds like it should, return settings for the toolkit. I've added a _check hook which will tell if the PHP functions needed are available. If _check fails, then the toolkit will not be listed on admin > settings and an error will be logged in watchdog. I've also update image.gd1.inc, image.imlib.inc, and image.imagick.inc with the same logic. ------------------------------------------------------------------------ March 28, 2005 - 23:08 : Junyor Attachment: http://drupal.org/files/issues/image.gd2.inc (3.07 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:09 : Junyor Attachment: http://drupal.org/files/issues/image.gd1_0.inc (1.8 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:09 : Junyor Attachment: http://drupal.org/files/issues/image.imagick.inc (2.19 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:10 : Junyor Attachment: http://drupal.org/files/issues/image.imlib.inc (2.07 KB) ------------------------------------------------------------------------ March 29, 2005 - 00:44 : gordon I am not too sure if the gd tool kit should be split into 2 tool kits. Now I am only speaking from a debian pov, but the php4-gd package is linked against gd2. Also from a Drupal POV, the difference between gd1 and gd2 are so minor that having them as they have always been, as effectively 1 tool kit has alway made sence from a maintenance POV. Also the standard php extension is just called gd. -1 on dividing gd1 and gd2 into separate tool kits.

Issue status update for http://drupal.org/node/19611 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Junyor Reported by: Junyor Updated by: Junyor -Status: by design +Status: patch @Gordon: image.inc specifically checks to make sure GD 2 is installed. It does not work with GD 1 as is. That's why Stefan had to make a separate GD 1 toolkit. @Walkah: GD 2 can still work out of the box with my design. We'd include image.inc and image.gd2.inc in the Drupal bundle, with the other toolkits available elsewhere. This change makes image.inc more future-proof, cleaner, and smaller. I don't see why that's a bad thing. In other words, this may be by design, but I think it's a bad design. Junyor Previous comments: ------------------------------------------------------------------------ March 28, 2005 - 22:46 : Junyor Attachment: http://drupal.org/files/issues/system-image.patch (6.8 KB) The new image handling tools in core have a lot of GD2-specific logic, making image.inc overly cluttered and complicated. If gd2 were just treated like another toolkit (though the only one included by default), then the image.inc code would be much cleaner and more easily maintained. This patch removes all the GD2 stuff from image.inc and puts it into image.gd2.inc. The _settings hook is now doing what it sounds like it should, return settings for the toolkit. I've added a _check hook which will tell if the PHP functions needed are available. If _check fails, then the toolkit will not be listed on admin > settings and an error will be logged in watchdog. I've also update image.gd1.inc, image.imlib.inc, and image.imagick.inc with the same logic. ------------------------------------------------------------------------ March 28, 2005 - 23:08 : Junyor Attachment: http://drupal.org/files/issues/image.gd2.inc (3.07 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:09 : Junyor Attachment: http://drupal.org/files/issues/image.gd1_0.inc (1.8 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:09 : Junyor Attachment: http://drupal.org/files/issues/image.imagick.inc (2.19 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:10 : Junyor Attachment: http://drupal.org/files/issues/image.imlib.inc (2.07 KB) ------------------------------------------------------------------------ March 29, 2005 - 00:44 : gordon I am not too sure if the gd tool kit should be split into 2 tool kits. Now I am only speaking from a debian pov, but the php4-gd package is linked against gd2. Also from a Drupal POV, the difference between gd1 and gd2 are so minor that having them as they have always been, as effectively 1 tool kit has alway made sence from a maintenance POV. Also the standard php extension is just called gd. -1 on dividing gd1 and gd2 into separate tool kits. ------------------------------------------------------------------------ March 29, 2005 - 00:51 : Junyor @Gordon: They already are separate toolkits. There's *no* GD 1 processing in core at all. This patch separates all the GD 2 specific functions from the general toolkit handling functions. The GD 1 toolkit attached here was provided by Stefan. ------------------------------------------------------------------------ March 29, 2005 - 01:22 : gordon You have missed what I am saying. In php there is no gd1 or gd2, where is just gd. which libary you use all depends on which one you link the gd extension against. So in the case of core, I think it was a bad move to remove all the gd1 code as it means that we require the gd module to linked against gd2 and will not work if it is not linked against gd1. ------------------------------------------------------------------------ March 29, 2005 - 02:57 : walkah Junyor : the gd support that is "bundled" with image.inc is meant only as a minimal "typical case" support for gd2 - so that image.inc will work out of the box in most installations (keep in mind that gd2 has been bundled with php 4.3 and that drupal 4.6 will require 4.3.3+). extra / additional toolkits are on an as-needed basis, and have already begun development - including a "gd2 advanced" that will make additional options available. the current behaviour, however, is very much by design (in fact, it was originally separated in my sandbox).

Issue status update for http://drupal.org/node/19611 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Junyor Reported by: Junyor Updated by: stefan nagtegaal Status: patch I tend to agree here.. We should separate api and code as much as possible, the same as text and markup. Imo this is a +1 from me, though I did not (yet) test this patch nor took a look at the coding styles... stefan nagtegaal Previous comments: ------------------------------------------------------------------------ March 28, 2005 - 22:46 : Junyor Attachment: http://drupal.org/files/issues/system-image.patch (6.8 KB) The new image handling tools in core have a lot of GD2-specific logic, making image.inc overly cluttered and complicated. If gd2 were just treated like another toolkit (though the only one included by default), then the image.inc code would be much cleaner and more easily maintained. This patch removes all the GD2 stuff from image.inc and puts it into image.gd2.inc. The _settings hook is now doing what it sounds like it should, return settings for the toolkit. I've added a _check hook which will tell if the PHP functions needed are available. If _check fails, then the toolkit will not be listed on admin > settings and an error will be logged in watchdog. I've also update image.gd1.inc, image.imlib.inc, and image.imagick.inc with the same logic. ------------------------------------------------------------------------ March 28, 2005 - 23:08 : Junyor Attachment: http://drupal.org/files/issues/image.gd2.inc (3.07 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:09 : Junyor Attachment: http://drupal.org/files/issues/image.gd1_0.inc (1.8 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:09 : Junyor Attachment: http://drupal.org/files/issues/image.imagick.inc (2.19 KB) ------------------------------------------------------------------------ March 28, 2005 - 23:10 : Junyor Attachment: http://drupal.org/files/issues/image.imlib.inc (2.07 KB) ------------------------------------------------------------------------ March 29, 2005 - 00:44 : gordon I am not too sure if the gd tool kit should be split into 2 tool kits. Now I am only speaking from a debian pov, but the php4-gd package is linked against gd2. Also from a Drupal POV, the difference between gd1 and gd2 are so minor that having them as they have always been, as effectively 1 tool kit has alway made sence from a maintenance POV. Also the standard php extension is just called gd. -1 on dividing gd1 and gd2 into separate tool kits. ------------------------------------------------------------------------ March 29, 2005 - 00:51 : Junyor @Gordon: They already are separate toolkits. There's *no* GD 1 processing in core at all. This patch separates all the GD 2 specific functions from the general toolkit handling functions. The GD 1 toolkit attached here was provided by Stefan. ------------------------------------------------------------------------ March 29, 2005 - 01:22 : gordon You have missed what I am saying. In php there is no gd1 or gd2, where is just gd. which libary you use all depends on which one you link the gd extension against. So in the case of core, I think it was a bad move to remove all the gd1 code as it means that we require the gd module to linked against gd2 and will not work if it is not linked against gd1. ------------------------------------------------------------------------ March 29, 2005 - 02:57 : walkah Junyor : the gd support that is "bundled" with image.inc is meant only as a minimal "typical case" support for gd2 - so that image.inc will work out of the box in most installations (keep in mind that gd2 has been bundled with php 4.3 and that drupal 4.6 will require 4.3.3+). extra / additional toolkits are on an as-needed basis, and have already begun development - including a "gd2 advanced" that will make additional options available. the current behaviour, however, is very much by design (in fact, it was originally separated in my sandbox). ------------------------------------------------------------------------ March 29, 2005 - 09:36 : Junyor @Gordon: image.inc specifically checks to make sure GD 2 is installed. It does not work with GD 1 as is. That's why Stefan had to make a separate GD 1 toolkit. @Walkah: GD 2 can still work out of the box with my design. We'd include image.inc and image.gd2.inc in the Drupal bundle, with the other toolkits available elsewhere. This change makes image.inc more future-proof, cleaner, and smaller. I don't see why that's a bad thing. In other words, this may be by design, but I think it's a bad design. ------------------------------------------------------------------------ March 29, 2005 - 09:42 : chx junyor, yesterday night Steven has fixed a very irritating bug in image module (variable_get...'gd2') so your system-image.patch won't apply. Please reroll.

Issue status update for http://drupal.org/node/19611 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Junyor Reported by: Junyor Updated by: walkah Status: patch well, i'll give Dries final word on this (as he has final say anyway) ... but it was also his idea to keep a "works for most cases" implementation in a single file. i like the _check idea, though what do usability guys say about not showing them? wouldn't it be more helpful to have them appear, but disabled with a message as to why they don't work? originally, the _settings was intended to do this kind of stuff in the form of form_set_error() -- and only for the selected toolkit (i.e. if you have other toolkits that are installed but don't work - who cares?). walkah Previous comments: ------------------------------------------------------------------------ March 28, 2005 - 16:46 : Junyor Attachment: http://drupal.org/files/issues/system-image.patch (6.8 KB) The new image handling tools in core have a lot of GD2-specific logic, making image.inc overly cluttered and complicated. If gd2 were just treated like another toolkit (though the only one included by default), then the image.inc code would be much cleaner and more easily maintained. This patch removes all the GD2 stuff from image.inc and puts it into image.gd2.inc. The _settings hook is now doing what it sounds like it should, return settings for the toolkit. I've added a _check hook which will tell if the PHP functions needed are available. If _check fails, then the toolkit will not be listed on admin > settings and an error will be logged in watchdog. I've also update image.gd1.inc, image.imlib.inc, and image.imagick.inc with the same logic. ------------------------------------------------------------------------ March 28, 2005 - 17:08 : Junyor Attachment: http://drupal.org/files/issues/image.gd2.inc (3.07 KB) ------------------------------------------------------------------------ March 28, 2005 - 17:09 : Junyor Attachment: http://drupal.org/files/issues/image.gd1_0.inc (1.8 KB) ------------------------------------------------------------------------ March 28, 2005 - 17:09 : Junyor Attachment: http://drupal.org/files/issues/image.imagick.inc (2.19 KB) ------------------------------------------------------------------------ March 28, 2005 - 17:10 : Junyor Attachment: http://drupal.org/files/issues/image.imlib.inc (2.07 KB) ------------------------------------------------------------------------ March 28, 2005 - 18:44 : gordon I am not too sure if the gd tool kit should be split into 2 tool kits. Now I am only speaking from a debian pov, but the php4-gd package is linked against gd2. Also from a Drupal POV, the difference between gd1 and gd2 are so minor that having them as they have always been, as effectively 1 tool kit has alway made sence from a maintenance POV. Also the standard php extension is just called gd. -1 on dividing gd1 and gd2 into separate tool kits. ------------------------------------------------------------------------ March 28, 2005 - 18:51 : Junyor @Gordon: They already are separate toolkits. There's *no* GD 1 processing in core at all. This patch separates all the GD 2 specific functions from the general toolkit handling functions. The GD 1 toolkit attached here was provided by Stefan. ------------------------------------------------------------------------ March 28, 2005 - 19:22 : gordon You have missed what I am saying. In php there is no gd1 or gd2, where is just gd. which libary you use all depends on which one you link the gd extension against. So in the case of core, I think it was a bad move to remove all the gd1 code as it means that we require the gd module to linked against gd2 and will not work if it is not linked against gd1. ------------------------------------------------------------------------ March 28, 2005 - 20:57 : walkah Junyor : the gd support that is "bundled" with image.inc is meant only as a minimal "typical case" support for gd2 - so that image.inc will work out of the box in most installations (keep in mind that gd2 has been bundled with php 4.3 and that drupal 4.6 will require 4.3.3+). extra / additional toolkits are on an as-needed basis, and have already begun development - including a "gd2 advanced" that will make additional options available. the current behaviour, however, is very much by design (in fact, it was originally separated in my sandbox). ------------------------------------------------------------------------ March 29, 2005 - 03:36 : Junyor @Gordon: image.inc specifically checks to make sure GD 2 is installed. It does not work with GD 1 as is. That's why Stefan had to make a separate GD 1 toolkit. @Walkah: GD 2 can still work out of the box with my design. We'd include image.inc and image.gd2.inc in the Drupal bundle, with the other toolkits available elsewhere. This change makes image.inc more future-proof, cleaner, and smaller. I don't see why that's a bad thing. In other words, this may be by design, but I think it's a bad design. ------------------------------------------------------------------------ March 29, 2005 - 03:42 : chx junyor, yesterday night Steven has fixed a very irritating bug in image module (variable_get...'gd2') so your system-image.patch won't apply. Please reroll. ------------------------------------------------------------------------ March 29, 2005 - 03:52 : stefan nagtegaal I tend to agree here.. We should separate api and code as much as possible, the same as text and markup. Imo this is a +1 from me, though I did not (yet) test this patch nor took a look at the coding styles... ------------------------------------------------------------------------ March 29, 2005 - 05:22 : Junyor Attachment: http://drupal.org/files/issues/system-image_0.patch (6.82 KB) Synched with HEAD.

Issue status update for http://drupal.org/node/19611 Post a follow up: http://drupal.org/project/comments/add/19611 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Junyor Reported by: Junyor Updated by: killes@www.drop.org -Status: patch (code needs review) +Status: patch (code needs work) Does not apply anymore. killes@www.drop.org Previous comments: ------------------------------------------------------------------------ Mon, 28 Mar 2005 21:46:05 +0000 : Junyor Attachment: http://drupal.org/files/issues/system-image.patch (6.8 KB) The new image handling tools in core have a lot of GD2-specific logic, making image.inc overly cluttered and complicated. If gd2 were just treated like another toolkit (though the only one included by default), then the image.inc code would be much cleaner and more easily maintained. This patch removes all the GD2 stuff from image.inc and puts it into image.gd2.inc. The _settings hook is now doing what it sounds like it should, return settings for the toolkit. I've added a _check hook which will tell if the PHP functions needed are available. If _check fails, then the toolkit will not be listed on admin > settings and an error will be logged in watchdog. I've also update image.gd1.inc, image.imlib.inc, and image.imagick.inc with the same logic. ------------------------------------------------------------------------ Mon, 28 Mar 2005 22:08:51 +0000 : Junyor Attachment: http://drupal.org/files/issues/image.gd2.inc (3.07 KB) ------------------------------------------------------------------------ Mon, 28 Mar 2005 22:09:13 +0000 : Junyor Attachment: http://drupal.org/files/issues/image.gd1_0.inc (1.8 KB) ------------------------------------------------------------------------ Mon, 28 Mar 2005 22:09:34 +0000 : Junyor Attachment: http://drupal.org/files/issues/image.imagick.inc (2.19 KB) ------------------------------------------------------------------------ Mon, 28 Mar 2005 22:10:01 +0000 : Junyor Attachment: http://drupal.org/files/issues/image.imlib.inc (2.07 KB) ------------------------------------------------------------------------ Mon, 28 Mar 2005 23:44:31 +0000 : gordon I am not too sure if the gd tool kit should be split into 2 tool kits. Now I am only speaking from a debian pov, but the php4-gd package is linked against gd2. Also from a Drupal POV, the difference between gd1 and gd2 are so minor that having them as they have always been, as effectively 1 tool kit has alway made sence from a maintenance POV. Also the standard php extension is just called gd. -1 on dividing gd1 and gd2 into separate tool kits. ------------------------------------------------------------------------ Mon, 28 Mar 2005 23:51:30 +0000 : Junyor @Gordon: They already are separate toolkits. There's *no* GD 1 processing in core at all. This patch separates all the GD 2 specific functions from the general toolkit handling functions. The GD 1 toolkit attached here was provided by Stefan. ------------------------------------------------------------------------ Tue, 29 Mar 2005 00:22:01 +0000 : gordon You have missed what I am saying. In php there is no gd1 or gd2, where is just gd. which libary you use all depends on which one you link the gd extension against. So in the case of core, I think it was a bad move to remove all the gd1 code as it means that we require the gd module to linked against gd2 and will not work if it is not linked against gd1. ------------------------------------------------------------------------ Tue, 29 Mar 2005 01:57:38 +0000 : walkah Junyor : the gd support that is "bundled" with image.inc is meant only as a minimal "typical case" support for gd2 - so that image.inc will work out of the box in most installations (keep in mind that gd2 has been bundled with php 4.3 and that drupal 4.6 will require 4.3.3+). extra / additional toolkits are on an as-needed basis, and have already begun development - including a "gd2 advanced" that will make additional options available. the current behaviour, however, is very much by design (in fact, it was originally separated in my sandbox). ------------------------------------------------------------------------ Tue, 29 Mar 2005 08:36:21 +0000 : Junyor @Gordon: image.inc specifically checks to make sure GD 2 is installed. It does not work with GD 1 as is. That's why Stefan had to make a separate GD 1 toolkit. @Walkah: GD 2 can still work out of the box with my design. We'd include image.inc and image.gd2.inc in the Drupal bundle, with the other toolkits available elsewhere. This change makes image.inc more future-proof, cleaner, and smaller. I don't see why that's a bad thing. In other words, this may be by design, but I think it's a bad design. ------------------------------------------------------------------------ Tue, 29 Mar 2005 08:42:15 +0000 : chx junyor, yesterday night Steven has fixed a very irritating bug in image module (variable_get...'gd2') so your system-image.patch won't apply. Please reroll. ------------------------------------------------------------------------ Tue, 29 Mar 2005 08:52:54 +0000 : stefan nagtegaal I tend to agree here.. We should separate api and code as much as possible, the same as text and markup. Imo this is a +1 from me, though I did not (yet) test this patch nor took a look at the coding styles... ------------------------------------------------------------------------ Tue, 29 Mar 2005 10:22:45 +0000 : Junyor Attachment: http://drupal.org/files/issues/system-image_0.patch (6.82 KB) Synched with HEAD. ------------------------------------------------------------------------ Tue, 29 Mar 2005 21:19:26 +0000 : walkah well, i'll give Dries final word on this (as he has final say anyway) ... but it was also his idea to keep a "works for most cases" implementation in a single file. i like the _check idea, though what do usability guys say about not showing them? wouldn't it be more helpful to have them appear, but disabled with a message as to why they don't work? originally, the _settings was intended to do this kind of stuff in the form of form_set_error() -- and only for the selected toolkit (i.e. if you have other toolkits that are installed but don't work - who cares?). ------------------------------------------------------------------------ Tue, 29 Mar 2005 22:14:27 +0000 : Junyor I thought about having the radio buttons visible, but disabled, though I couldn't figure out how to selectively disable the radio buttons. The current implementation shows an error after selecting a toolkit that won't work. I'd rather not show it at all (and show an error in the logs).