Project page - http://drupal.org/sandbox/thedavidmeister/1672224
$ git clone http://git.drupal.org/sandbox/thedavidmeister/1672224.git
Comments by me on other sandboxes under review:
- http://drupal.org/node/1694474#comment-6260750
- http://drupal.org/node/1696494#comment-6260824
- http://drupal.org/node/1684700#comment-6260890
This module is for both Drupal 6 and 7 but since ImageCache was merged into core in D7 it works with the core's "Image" module instead of ImageCache there.
Image(Cache) Defaults
This module allows for default images to be configured for ImageCache presets and/or Image styles. This module is intended to address two primary use cases:
- The intended or implemented layout of a site relies heavily on images for its structure. Often it becomes difficult or disproportionately time consuming to maintain these sites in a way that degrades gracefully when those images are absent.
- A developer working locally with an imported codebase or distribution profile wants to create a development instance of a production site by (re)importing a copy of the database but she wants to avoid dealing with the cumbersomely large files directory or the inappropriate/incomplete content generated by the Devel Generate module. Avoiding both can easily lead her to introduce new layout issues that aren't detected until after deployment (which is bad) simply because she isn't working with an accurate enough representation of the production environment.
This module is intended for developers who want to minimise that frustrating time spent importing and deploying projects with large files directories that contain mostly images. We do our best here to make sure this module works "out of the box", but still provides intelligent configuration options and is 100% exportable if you're working with Features and Distribution Profiles.
What Image(Cache) Defaults currently does
- Scans "sites/[site]/files", "sites/all/files", "profiles/[current_profile]/files", "path/to/imagecache_defaults/images" (in that order) for an image named "imagecache_defaults" and sets this as the default image for Image(Cache).
- Detects if a broken file-path is about to be sent to Image(Cache) to be rendered and replaces it with the default image
- Works equally well with ImageCache presets in Drupal 6 and core Image styles in Drupal 7
- (Optional) Allows you to apply a preset/style to the default image in addition to the preset/style that would be applied to the broken image
- (Optional) Logs a watchdog message to the database when broken file paths are detected
- (Optional) Remove all broken images from your markup completely to avoid 404 errors instead of replacing them with the default
Alternatives
For an alternative way to work around broken image links, have a look at Stage File Proxy (SFP) which allows you to automatically point your broken image links at a custom url but at the time of writing it has a few limitations that ImageCache Defaults doesn't:
- SFP doesn't check if certain files exist before replacing them so you can accidentally DOS yourself with it #1545912: stage_file_proxy should check if the file already exists, even if it's not an imagecache
- SFP can't remove broken image tags from your page completely
- SFP isn't recommended for use on production sites so there's an extra step in downloading, enabling and configuring it every time you reimport your production site
- SFP doesn't log the location of broken images to watchdog
- SFP relies on server configuration but has only been tested on Apache 2 on Mac OSX and Linux
- SFP requires the developer to have the same relative paths to system file directories on development and production sites
- SFP requires more configuration than simply enabling it before it starts working
Comments
Comment #1
patrickd commentedwelcome,
delete your master branch:
Don't use 0, 1 as booleans - Use TRUE or FALSE.
just had a quick glance but beside this your module looks really good
Comment #1.0
thedavidmeister commentednew link to project comment
Comment #2
thedavidmeister commentedI was a bit unsure about that... I did it where the checkbox values get saved in the database as 0 or 1 so I mirrored that in my code. It seems strange to me that the variable would be 1 when set and ticked, 0 when set and unticked but FALSE when it isn't set, but I'm happy to use boolean values for defaults since it all behaves the same in the end.
I'll clean out the master branch now. I'll push up revisions in half an hour or so.
Thanks for the prompt review! I'm also working through other projects atm to get the bonus :)
Comment #3
thedavidmeister commentedAight, I've emptied out master and swapped out my 0s for FALSEs in both 6.x-1.x and 7.x-1.x
Comment #3.0
thedavidmeister commentedadded a link to a project review
Comment #4
thedavidmeister commentedAdding review bonus tag. Here are my reviews:
- http://drupal.org/node/1694474#comment-6260750
- http://drupal.org/node/1696494#comment-6260824
- http://drupal.org/node/1684700#comment-6260890
Comment #5
cthiebault commentedIn imagecache_defaults.module, line 87:
You defined
$scan = array();but it is overwritten 5 lines latter$scan = file_scan_directory($dir, $mask, $options);Line 269:
Parameter
$uriis never used butimagecache_defaults_valid_image_uri($path);uses$paththat is not defined.Comment #6
thedavidmeister commentedah, good catch on the path/uri thing! I'll revise that in the next few days. Moving this to needs work until I make those changes.
Comment #7
thedavidmeister commentedI've fixed and pushed up the superfluous variable setting in 6.x-1.x and 7.x-1.x and the incorrect variable names in 7.x-1.x
Setting this back to "needs review" :)
Comment #8
klausimanual review of the 7.x branch:
"global $profile;": why do you need that as a global variable? variable_get() is cached anyway, so there is no performance gain.
But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Comment #9
thedavidmeister commentedthe "global $profile" you're referencing is taken from Libraries. Apparently variable_get() won't give you anything useful during site installation. I couldn't think of a time you'd come across image styles during site installation during "normal" usage of Drupal but I figured that one line could deal with any unforseen edge-cases like that nicely.
Comment #10
sdboyer commentedyep, this looks good. promoting thedavidmeister.
Comment #11
thedavidmeister commentedthis review process was really smooth, much appreciated the effort from everyone who looked over my work :) - http://drupal.org/project/imagecache_defaults
Comment #12
patrickd commented@sdboyer, please have a look at proper text templates at http://groups.drupal.org/node/184389 when promoting somebody.
Here are some recommended readings to help with excellent maintainership:
You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!
Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
Thanks to the dedicated reviewer(s) as well.
Comment #13
thedavidmeister commentedThanks patrickd. I found a few of those while googling around but it's nice to have them summarised. I think I'll definitely have to get better at giving my commits good names because I've been pretty... liberal... with my wording so far, got that page bookmarked now :)
Comment #14
klausi@sdboyer: thanks for helping out here! I added you to the list of code review administrators: http://groups.drupal.org/node/142454
Comment #15.0
(not verified) commentedadding link to other project review comment