Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
We discovered that the imagefield_crop module poorly stores the crop info data inside the variables table. Crop information is per file. This creates a direct connection between the number of managed images in the system and the size of the variable in the variables table.
One of my sites has over 43,000 images with imagefield crop settings stored making this the largest variable in the variables table by a factor of 3000. Because the entire varibles table is loaded out of cache on every page load, this is not a scalable storage method.
Proposed resolution
Save the crop information in a data table. I'v attached a patch that creates the table and converts the variable into rows within the table.
API changes
The patch introduces two new functions for retrieving and saving cropping information. I tried using the entity load hook but it didn't work for some reason.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | imagefield_crop-db_storage-1993144-15.patch | 2.9 KB | joetsuihk |
Comments
Comment #1
joetsuihk CreditAttribution: joetsuihk commented+1 for the idea, thanks or the effort. But I cannot apply the patch by `git -v apply [patchname]`
Should it applying to 7.x-1.1 or 7.x-1.x?
I am still unable to get this after manually apply the patch
I few suggestions:
rename imagefield_crop_file_settings() into imagefield_crop_file_settings_load()
in imagefield_crop_file_settings(), fetchCol() should be replaced by fetchAssoc()? Why you only need first column?
Comment #2
Josh Waihi CreditAttribution: Josh Waihi commentedYeah you're right, I did need to use fetchAssoc instead of fetchCol (See new patch attached).
Comment #3
Josh Waihi CreditAttribution: Josh Waihi commentedBTW: patch applies fine with drush make (how I'm tracking the change).
Comment #4
zhgenti CreditAttribution: zhgenti commentedHi guys, 7.x-2.x was originally started to handle this issue with storing data in the variables table and other issues. I think what really would be great is to create an update path from 7.x-1.x to 7.x-2.x. I didn't have enough time to add this, but it's really worth doing it. I also thinking about 3.x version and going to create issue for discussion.
Cheers,
Dmitry
Comment #5
joetsuihk CreditAttribution: joetsuihk commentedDuplicate #1566060: Storing crop data in a variable doesn't scale well.. There is a patch from 2012 May also available for review. Heop we can combine the effort somehow.
Comment #6
reubenavery CreditAttribution: reubenavery commentedHere's the same patch re-rolled against latest dev.
Upping the priority and changing this to bug report. This is a huge deal for sites with any significant number of nodes with cropped imagefields, and in my opinion is definitely a bug, not a feature request. Expecting variable_get and PHP to be a database is really really wrong.
Comment #7
wiifmOkay, had a look at the most recent attached patch, and have made a few changes:
dpm()function calls (why are these even in there?)Have tested the patch (used to have 35KB of data in the one variable imagefield_crop_info and now I have none), and all functionality appears intact, the update hook successfully migrated the data to the new schema.
Really happy with this approach, seems sane, and reduces a lot of deserialize PHP calls on every page request.
Marking as RTBC as my patch only enhances the previous patch by @reubenavery. Feel free to chime in with a +1 if this patch works for you too.
Comment #8
joetsuihk CreditAttribution: joetsuihk commentedThanks @wiifm ! As this is an important change, involves database schema changes, I suggest we have one more reviewer before I commit this to dev. Marked the issue need review so others can help on this.
Comment #9
grota CreditAttribution: grota commentedRerolled patch from https://drupal.org/node/1993144#comment-8269865 onto latest 7.x-1.x branch (sha
9c1653aas of today)Comment #10
valthebaldWorks like a charm for me
Comment #11
Bastlynn CreditAttribution: Bastlynn commentedWorks perfectly on this end too. DB tables migrated fine.
Comment #12
Alex Bukach CreditAttribution: Alex Bukach commentedRerolled patch from #9 for latest 7.x-1.x head.
Comment #13
Bastlynn CreditAttribution: Bastlynn commentedStill good...
Comment #14
Bastlynn CreditAttribution: Bastlynn commentedWe ready to roll on this?
Comment #16
joetsuihk CreditAttribution: joetsuihk commentedYes!
This is the latest patch i applied, #12 is a bit old.
commited to 7.x-1.x http://cgit.drupalcode.org/imagefield_crop/commit/?id=6d5bf60
Thanks everyone for the effort!
Comment #17
joetsuihk CreditAttribution: joetsuihk commentedmark as fixed
Comment #18
Bastlynn CreditAttribution: Bastlynn commentedwoot! Thanks! :)