new IM driver for Image_Transform

Peter, Great, I am siked that you have followed up. I will try to be as clear as possible in this reply and perhaps we can move forward with the decisions here. Pear-dev, Sorry this looks broken up, but I figured the discussion should be made public...I quoted Peter in many cases on questions referring the Image_Transform, particular the IM driver > What is the advantage of this rather than using a defined constant? AFAIK > which() won't work on Windows? (using CMD::which() for path to > identify and convert) Well, it should...the advantage is for autodetecting (good) instead of using a constant (bad). It is common in PEAR to try to autodetect the environment...and frees the user from having to know that imagemagick is being used from the commandline... > >- always use escapeshellcmd() and escapeshellarg() whenever possible > Yes, I've been meaning to ad them but wasn't sure if it was really > necessary - after all this class should _never_ accept user input. Not just for that, you might have a filename with quotes or something and if you try to save it will blow up. It is good to do this so that you eliminate trivial commandline errors. > Why not use passthru over exec? I used passthru so that any error > message printed to sdout would be echoed to screen for the user. This is definitely not the PEAR standard...when running a class, errors should always be passed to PEAR's error handler and never echoed directly to screen. Imagine a production environment where the end user is seeing something like 'convert' [Unknown arguments] Very bad...we should try to capture every last bit of error output and handle it appropriately...Using the commandline is very fragile unless this is done, since it is a child process and it is easily to write a program that can silently fail. > it so that we can support cURL etc for fetching the images - I have remote > fopen calls disabled and use cURL instead on my server. Fine, either way...autodetect of course the best method...point is it should be able to do this since the GD extension can. > I don't believe that this is necessary (error checking). If > there's an error in the usage of the class then the person will > find out when: > a) they view the image > b) when a PHP error message > is printed to the screen This is very non-PEAR standard...PEAR has always held validation sacred and has done everything it can to check even programming errors...If you believe it slows down the class, then talk to the PEAR designers because regardless of speed, they believe importance outweighs this...and with PEAR, nothing should be printed to php://stdout unless PEAR is instructed to do so through the Error Interface. > No need IMO to slow down the classes more than we have to :-) It really doesn't slow it down at all, since php is so fast at these trivial checks. > I can't find CMD.php anywhere in PEAR :-( It is in the original cvs tree in /php4/pear/CMD.php...it will be moved soon, but until it is...just require it there. > Does the function factory() need to be passed by reference? Just intrigued as I > haven't bothered to do this before. Yes, since an object is created internally and then returned...you pass by reference so that all calls on it will work with the original object and not a copy....check out the php manual, they refer to this exact thing...I got the idea for this function from Horde (great examples of php PEAR classes there) and they pass it by reference. > IIRC we're changing the name from ::setup to ::factory for consistency or > is there any other reason? To me, setup is more descriptive (init would be > even better), but if the rest of PEAR is going to use ::factory then we'd > better. PEAR standard as it is used elsewhere, such as in Auth...I don't really care what the name is, as long as it contains the code I had suggested...namely including the driver file and instantiating the class. > >register deconstructors for open images in GD: > Good thinking. However, what would happen if the deconstructor is > registered and someone calls destroy() - what does the deconstructor > do? Try and free a non-existent part of the memory? Crash? Naturally it only destroys what hasn't been destroyed, and each element is cleared from the list when destroy() is explicitly called. > I found out why ImageMagick gives me out of memory errors: every process on > my server is limited in size to 14Mb. Thank you very &%#*@ much Verio! I, hence the call to ulimit (which you had mistakenly put as unlimit, at least there is no such Unix shell function unlimit)...I am not sure this call is necessary...plus ulimit seems to print to stdout...anyway, I am not 100% on that last paragraph, no real opinions. > Thanks for the feedback! Hope I don't sound too critical - I'm trying to > get a grip on the ideas and why they'd be good before I make any changes... Totally understand...thanks for the class! Dan -- ________________________________________________________________ Daniel Allen, <dan@mojavelinux.com> http://www.mojavelinux.com/ ________________________________________________________________ It is not enough to succeed. Others must fail. -- Gore Vidal ________________________________________________________________