First I need to issue my great thanks for this awsome module! A menu callback to an existing directory! Genius! Genius, I say! I can think of several scenarios where I'll use this technic in the future. But now... on to problems:
Right now the admin page throws a PHP error and has a lot of duplicate code in weird places (like a cut and paste gone horribly wrong). Hence the 'critical'. Beyond that issue, everything below is normal priority.
I've attached a patch which applies to the 4.7 branch. It makes numerous changes to admin side interface and behavior. Here's the complete list of changes:
Functional:
- Split up imagecache_admin_list function into proper form-handling functions (imagecache_admin_validate and imagecache_admin_validate). This eliminates the need to do check_plain() everwhere since the Forms API handles it all for us.
- Add a small amount of validation on ruleset names (only alphanumeric, _, and -)
- Removed auto_increment from DB schema, instead using the Drupal sequences table and db_next_id() (requires update.php run)
Visual:
- Drastic reduction of buttons and fields (screenshots to follow)
- Only one imagecache ruleset is displayed at a time (the one being edited/created)
- Added help text for imagecache actions menu
I think that covers the set of changes. I know that's a lot of changes all at once, but if I seperate it out the patches conflict with each other, so I thought it was better this way.
I'm using imagecache (or plan on using) for several high-volume sites, it's crucial that we make this module run butter-smooth. Thanks again! This module rocks!
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | imagecache_admin.patch | 20.76 KB | quicksketch |
| #2 | imagecache_after.png | 83.51 KB | quicksketch |
| #1 | imagecache_before.png | 176.58 KB | quicksketch |
| imagecache_admin_patch.zip | 260.61 KB | quicksketch |
Comments
Comment #1
quicksketchBefore screenshot
Comment #2
quicksketchAfter
Comment #3
dopry commentedThe visual updates are awesome. I just read through the code, can you make a patch against the 4.7 tree? I dropped the term ruleset and replaced it with preset... I thought it was a little easier for people to get. Also could you split dropping auto_increment into another patch? It doesn't really belong in a UI clean up, although I'd like to get it in asap.
I'll be running my issue queue again on tuesday I think.
.darrel.
Comment #4
quicksketchYep, can do. I'll split thing up as best as possible. I should have mentioned the preset/ruleset thing. I noticed that the naming was inconsistent so I went with the one in the DB schema.
While making the patch for auto-increment I can also make a ruleset->preset patch since we'll be requiring people to run update.php anyway. Awesome, can't wait to get these changes in.
Comment #5
quicksketchHere's a new patch which reduces the scope of the changes. I've left the preset/ruleset terminology as much as possible (another patch will follow to fix that inconsistency after this patch is applied) and moved the auto_increment patch to another issue: http://drupal.org/node/89231.
If you can apply quickly that'd be awesome. I want to make several more changes, but just getting these two patches not to overlap nearly spaghetti-brained me. Also, the current admin page in 4.7 branch is totally unusable with it throwing that whitescreen PHP error, so getting that fixed (which this patch does) would be great.
Comment #6
quicksketchComment #7
dopry commentedcommited as 1.9.2.5
fixed some issues with _imagecache_recurive_delete being called with a non-existant path, move flush calls to before db_updates so ruleset names wouldn't change before flus occured, and got rid of the word ruleset completely.
Comment #8
quicksketchYay, thanks so much! I'm glad you caught that recursive problem, it tried to delete my HD a couple times! Hang on with my other patch, I've got an update to issue. (**Hurrying to the issue queue**)
Comment #9
dopry commentedArg, already committed it... I did catch two bugs in it, and made the changes necessary for it to work with my nomenclature changes. I would appreciate it if you could put imagecache through its paces, I've really only focused on testing the admin interfaces.
Comment #10
(not verified) commented