Refactoring image handling in controller

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

Post Reply
User avatar
allspiritseve
DevNet Resident
Posts: 1174
Joined: Thu Mar 06, 2008 8:23 am
Location: Ann Arbor, MI (USA)

Refactoring image handling in controller

Post by allspiritseve »

I made a little class to handle images, resizing and stuff. I also made a little factory that takes an image in $_FILES and turns it into an Image object. I also made a little gateway that handles inserting a record into the db for each iamge. And yet... I don't even have error handling yet and my code looks like this:

Code: Select all

       if (strlen ($_FILES['image']['name']) > 0)    {
            $image_data = $this->imagesGateway->insert(array ('filename' => ''));
            $image_data['filename'] = 'images/image' . $image_data['id'] . '.jpg';
            $this->imagesGateway->update ($image_data['id'], $image_data);
            $image = $this->factory->create ($_FILES['image']);
            $image->saveJpeg ('../' . $image_data['filename']);
            $_POST['image_id'] = $image_data['id'];
        } elseif ($_POST['remove_image'])    {
            $this->imagesGateway->delete ($page->image_id);
            @unlink ('../images/image' . $page->image_id . '.jpg');
        }
        $this->pagesGateway->update ($page->id, $_POST);
I have never seen a clean way of handling images right from the request... most examples I see are using PHP functions and have no abstraction whatsoever.

I feel like this is way too much for the controller to handle. I would love it if I could refactor out some code into something like this:

Code: Select all

       if ($this->files->get ('image'))    {
            try {
                $image = $this->imagesGateway->save ($this->files->get ('image'));
            } catch (Exception $e)    {
                $this->addError ($e->getMessage());
            }
            $page->add ($image);
            $pagesGatway->update ($page->id, $page)
        }
Aside from just copying the code above, I have a lot of design decisions to make. Should I have an intercepting filter that watches for images in $_FILES and makes Image objects available in the controller? If so, should the intercepting filter be looking for errors and providing them to the controller as well? If there was an upload error I can't create an image handle from the image, so it would have to have at least a small amount of error handling.

And then, I've got two separate places I need to save that image: physically in the filesystem, preferably with a name that links it to a record in the database ('images/image60.jpg') and then I need to save in the database, preferably with the whole url. I could make two separate gateways, but as you can see these overlap. I could make one gateway... but I worry that I won't have enough control at that point. What if I need to save an image in another location? What if I need images saved in a different part of the database?

In short, I feel like there are things I can be doing to make this controller much lighter, but I'm not really sure what those things are...
alex.barylski
DevNet Evangelist
Posts: 6267
Joined: Tue Dec 21, 2004 5:00 pm
Location: Winnipeg

Re: Refactoring image handling in controller

Post by alex.barylski »

For starters, I would only put something in an intercepting filter (a name which I dislike I prefer front controller pre/post-dispatch plugins -- as they can do a lot more than 'filtering') if it's truly global. Checking for an $_FILES existance surely does not occur on every request?

Secondly, much of your code could probably be moved into a model object. Controllers should basically only provide the wiring and such. Validation, uploading, creating, deleting, etc should be done at the model level. Doesn't have to be and sometimes it might make sense to keep it in the controller but eventually you will refactor into a model so I always try and just go with that approach.

Without getting into code specifics:

1. Call a method on the model called uploadImage() or similar. Pass in the path to the temp file (and the path for the destination maybe).
2. Check if the file is of type image (validation) and within accepted dimensions.

Basically pass in all controller context (ie: GPC) but move the code into a model method.

Make the model flexible enough to possibly accomodate all all file uploading assuming you use a single table for all file types, otherwise have a method for each file type, etc.

Cheers,
Alex
User avatar
inghamn
Forum Contributor
Posts: 174
Joined: Mon Apr 16, 2007 10:33 am
Location: Bloomington, IN, USA

Re: Refactoring image handling in controller

Post by inghamn »

I use an ActiveRecord style Media class for handling all the database and file manipulation for images. Doing that work in the model allows the controller to just focus on handling the request and creating and saving the models.

Controller code would look something like:

Code: Select all

 
if (isset($_FILES['userfile']) {
    $image = new Media();
    $image->setFile($_FILES['userfile']);
    $image->save();
}
 
Basically, let the Model worry about how and where to save stuff.
User avatar
allspiritseve
DevNet Resident
Posts: 1174
Joined: Thu Mar 06, 2008 8:23 am
Location: Ann Arbor, MI (USA)

Re: Refactoring image handling in controller

Post by allspiritseve »

PCSpectra wrote:For starters, I would only put something in an intercepting filter (a name which I dislike I prefer front controller pre/post-dispatch plugins -- as they can do a lot more than 'filtering') if it's truly global. Checking for an $_FILES existance surely does not occur on every request?
For the front end, it probably wouldn't make sense... but file and image uploading are fairly common, and if I've got 5-10 different forms that need images or files, that's 5-10 times that code is duplicated. It's not like checking whether $_FILES is empty is going to take up all that much processing time anyways.
PCSpectra wrote:Secondly, much of your code could probably be moved into a model object. Controllers should basically only provide the wiring and such. Validation, uploading, creating, deleting, etc should be done at the model level. Doesn't have to be and sometimes it might make sense to keep it in the controller but eventually you will refactor into a model so I always try and just go with that approach.
I feel like the code I posted is pretty much all wiring though. As I said, I haven't gone through and put in validation yet, this really just the barebones-fail-miserably-on-error code.
inghamn wrote:I use an ActiveRecord style Media class for handling all the database and file manipulation for images. Doing that work in the model allows the controller to just focus on handling the request and creating and saving the models.
That's kind of where I was going with the filter/plugin... except for the save part. If the image object were already provided to the controller, I could almost one-line it.
inghamn wrote:Basically, let the Model worry about how and where to save stuff.
The how I agree with... I don't know so much about the where. It seems kind of restricting to not have the flexibility to save images somewhere else if I need to. Maybe that's something that I need defaults for, and I can modify when necessary.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Refactoring image handling in controller

Post by Christopher »

allspiritseve wrote:I have never seen a clean way of handling images right from the request... most examples I see are using PHP functions and have no abstraction whatsoever.
I think upload is rare enough that it is usually a custom implementation.
allspiritseve wrote:I feel like this is way too much for the controller to handle. I would love it if I could refactor out some code into something like this:
That does not seem like so much. The goal is thinner controllers -- it is not a law that above 12 lines is wrong.
allspiritseve wrote:Aside from just copying the code above, I have a lot of design decisions to make. Should I have an intercepting filter that watches for images in $_FILES and makes Image objects available in the controller? If so, should the intercepting filter be looking for errors and providing them to the controller as well? If there was an upload error I can't create an image handle from the image, so it would have to have at least a small amount of error handling.
I don't see how taking a slightly heavier controller and putting its code in a pre-filter makes the solution any lighter? It is just hiding the controller code from view.
allspiritseve wrote:And then, I've got two separate places I need to save that image: physically in the filesystem, preferably with a name that links it to a record in the database ('images/image60.jpg') and then I need to save in the database, preferably with the whole url. I could make two separate gateways, but as you can see these overlap. I could make one gateway... but I worry that I won't have enough control at that point. What if I need to save an image in another location? What if I need images saved in a different part of the database?
I have a couple of thoughts. One is that it might be nice if a Request object could create a Files object that did the checks you mention. The Request seem the logical place for it. If it only created the Files object on demand then you would not have the overhead of seldom used code every request.

As far as having the file and database functionality in one object. It seem like perhaps you should create a Facade to these subsystem so that the could change underneath, but the interface in the controller is always the same.

I am interested in this Files object. What kinds of checking, organization and abstraction would it do?
(#10850)
User avatar
Eran
DevNet Master
Posts: 3549
Joined: Fri Jan 18, 2008 12:36 am
Location: Israel, ME

Re: Refactoring image handling in controller

Post by Eran »

I usually have the models handle images. It's not uncommon for the same handling procedure to be used in several places in the system, and also since often I need to save the image name in the database, which is also the responsibility of the appropriate model.
User avatar
allspiritseve
DevNet Resident
Posts: 1174
Joined: Thu Mar 06, 2008 8:23 am
Location: Ann Arbor, MI (USA)

Re: Refactoring image handling in controller

Post by allspiritseve »

arborint wrote:I think upload is rare enough that it is usually a custom implementation.
Is it really that rare? Maybe it's just that I'm doing small CMS-driven sites...
arborint wrote:That does not seem like so much. The goal is thinner controllers -- it is not a law that above 12 lines is wrong.
I'm not so worried about 12 lines, but duplicating those 12 lines in many different controllers seems to be a good indication of a need for some abstraction.
arborint wrote:I don't see how taking a slightly heavier controller and putting its code in a pre-filter makes the solution any lighter? It is just hiding the controller code from view.
If it's in a prefilter, then the controller doesn't even need to worry about constructing an object from $_FILES, it would just work on the object and any error messages that may have arisen. Might only save me 8 lines of code on one controller, but between 10 controllers I'm saving 80 lines.. etc.
arborint wrote:One is that it might be nice if a Request object could create a Files object that did the checks you mention. The Request seem the logical place for it. If it only created the Files object on demand then you would not have the overhead of seldom used code every request.
The request object does seem a logical place for it. That's what I was thinking the filter could do... checking for $_FILES and only creating objects if needed.
arborint wrote:I am interested in this Files object. What kinds of checking, organization and abstraction would it do?
Well, $_FILES can produce a pretty ugly associative array, especially if you're using brackets to upload multiple images at the same time. Also, apparently if you use brackets in the name, it will produce an array if $FILES['filename'][0], etc. rather than $_FILES[0]['filename']. That's ugly to sort through, and it would be nice if my controllers could be given an array of objects to work with rather than dealing with that mess. Potentially, I could have it check whether images are of the correct filetype, the right size, etc. I don't know if that belongs there or not, though.
User avatar
allspiritseve
DevNet Resident
Posts: 1174
Joined: Thu Mar 06, 2008 8:23 am
Location: Ann Arbor, MI (USA)

Re: Refactoring image handling in controller

Post by allspiritseve »

Alright, I've fiiinally got some time to rewrite my image handling code.

What is an Image object? It seems like there's different requirements for this object in different situations. Currently my image object is a GD handle that I can use to modify an image, and save it to the database. That by far saves me the most code. In order to clean up some of the remaining code, I am in need of an object that wraps a physical location of an image as a field in the database, and also has alt text, width, height, and potentially caption text. This object will be used in templates to display a specific image.

But what if I need different sizes of that image in different areas? Is there one image object per original image, with a naming convention to create/retrieve different sizes? I have worked on sites where the user wanted control of their own thumnnails. I'm not sure if that should still be considered the same image or not. Finally, I also need extra data wrapping an image-- for instance, using a image in a gallery needs a title, description, sort value, association to various galleries, etc.

Should all this be the same object? Maybe a core object with some wrappers? Any wisdom on the subject that can help me clear up my thinking would be much appreciated.
User avatar
inghamn
Forum Contributor
Posts: 174
Joined: Mon Apr 16, 2007 10:33 am
Location: Bloomington, IN, USA

Re: Refactoring image handling in controller

Post by inghamn »

For my stuff I've added all the image handling functions into the same class that interacts with the database. I still only store the filename in the database, though. Here's my family's photo album using this software, it's currently got about 6,000 photos in it. A couple hundred more on the way (my brother just got married)

http://cliffandjulie.net/photobase/

It's been a pet project of mine for the past 10 years. I've also used the Media class from this project in my work's CMS system, and basically any other application that needs image handling. All of the image handling code adds up to about 500 sloc. That's database calls, file handling, and image manipulations.

Here's the Media class:
https://sourceforge.net/projects/photodatabase/
http://photodatabase.svn.sourceforge.ne ... iew=markup



I can make some general recommendations on building your own image handling.

GD vs ImageMagick
There's really no contest...ImageMagick produces better quality images when doing manipulations and uses less CPU and memory when doing it. I have not tried out the new PHP IMagick extension, yet. PHP's exec() calls are fine with me. (Do NOT do the exec() calls with any variables from the user.)
ImageMagick via exec() means you can batch process tons of photos at once, outside of PHP.



Image Sizes:
With a known filename and ImageMagick you don't have to store the width and height in the database. Just look at the image file directly each time you need it.

I generate and cache the smaller (thumbnails, icons) sizes as needed using ImageMagick. The files are stored on the hard drive in a directory structure that makes it easy to find an image by hand, if needed.

Code: Select all

 
/2008/09/12/23.jpg
/2008/09/12/thumbnails/23.jpg
/2008/09/12/icons/23.gif
 

Apache:
Serve the images with URLs that point directly to the image file. Do not even think about using PHP to do stuff like fopen() fread() on the images. PHP will spend the bulk of it's CPU time just streaming image data to the user - something which Apache is much better suited.

Use a custom 404 handler for images. That way, if an expected size (thumbnail, icon, etc) doesn't exist, you can fire off PHP code to generate and save the custom size.

Code: Select all

 
<Directory "/path/to/application_name/media">
    ErrorDocument 404 /photobase/media/404.php
</Directory>
 
Here's the 404.php

Code: Select all

 
if (preg_match('|/([a-z]+)/([0-9]+)\.[a-z]{3}$|',$_SERVER['REQUEST_URI'],$matches))
{
    $_GET['size'] = $matches[1];
    $_GET['media_id'] = $matches[2];
 
    try { $media = new Media($_GET['media_id']); }
    catch (Exception $e) { $_SESSION['errorMessages'][] = $e; }
 
    include APPLICATION_HOME.'/html/media/source.php';
}
else { $_SESSION['errorMessages'][] = new Exception('media/unknownMedia'); }
 
Header('http/1.1 404 Not Found');
Header('Status: 404 Not Found');
$template = new Template();
echo $template->render();
 
And here's the source.php that handles streaming the image back to the browser. Remember don't use this to serve all your images. But it is possible, and works pretty well for streaming newly generated sizes - instead of redirecting the browser again.

Code: Select all

 
$media = new Media($_GET['media_id']);
$size = isset($_GET['size']) ? $_GET['size'] : '';
 
foreach(Media::getSizes() as $knownSize=>$sizeInfo)
{
    if ($size == $knownSize)
    {
        $extensions = Media::getExtensions();
        Header('Content-type: '.$extensions[$sizeInfo['ext']]['mime_type']);
        $media->output($size);
        exit();
    }
}
 
# If we could not find the size they asked for, just show the medium size
Header("Content-type: image/jpg");
$media->output('medium');
 
Post Reply