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.
If I export the image styles that have the cropping dialog to a feature,
and on the production site I revert the styles to the feature code,
the cropping dialog doesn't let me switch between the different styles.
If I set the image styles to overridden status it works, but this is really not desirable on production environment.
Comment | File | Size | Author |
---|---|---|---|
#30 | epsacrop-fix_update_hook-1396500-30.patch | 3.71 KB | coredumperror |
#22 | crop_style_from_code-1396500-22.patch | 4.71 KB | nagy.balint |
#21 | crop_style_from_code-1396500-21.patch | 6.51 KB | dagomar |
#19 | crop_style_from_code-1396500-19.patch | 5.45 KB | olofbokedal |
#17 | crop_style_from_code-1396500-8-v2.patch | 4.57 KB | mattsmith3 |
Comments
Comment #1
pmelab CreditAttribution: pmelab commentedThe dialog relies on "ieid" and "isid" database fields, which are not present if the style is loaded from code instead. There's got to be another unique identifier. Perhaps style-name, width and height?
Comment #2
olofbokedal CreditAttribution: olofbokedal commentedCreated a patch that will rely on the style name and effect id instead. Please try it out to see if this will work for you as well.
Comment #3
sebas5384 CreditAttribution: sebas5384 commentedHey! the patch #2 it works! thanks @ojohansson
But! there is a problem, all the crops already made with the Epsa Crop, ins't working anymore.
Thats a really big problem. I'm going look into it a little more deep, to see if i can help :)
Cheers,
Sebas.
Comment #4
sebas5384 CreditAttribution: sebas5384 commentedComment #5
sebas5384 CreditAttribution: sebas5384 commentedIt seems that this coordinate isn't right:
{"epsacrop--":{"x":15,"y":12,"x2":340,"y2":243,"w":325,"h":231}}
Its really difficult to tell wich id of effect and preset's machinename belongs to.
I think that an hook_update_N() with some very heavy updates querys to the epsacrop_files table.
For example:
Before:
{"epsacrop--":{"x":15,"y":12,"x2":340,"y2":243,"w":325,"h":231}}
After:
{"epsacrop-6-product_full":{"x":15,"y":12,"x2":340,"y2":243,"w":325,"h":231}}
To me, this seems to be the only way to fix the problem, anyone haves some other ideas?
Cheers,
Sebas.
Comment #6
olofbokedal CreditAttribution: olofbokedal commentedThe hook_update_N() way is the way to go. Every stored crop has to be updated to match the new identifier, which has changed from
epsacrop-EFFECT_ID-STYLE_ID
toepsacrop-EFFECT_ID-STYLE_NAME
.Comment #7
sebas5384 CreditAttribution: sebas5384 commentedI'm trying to find a solution to make features work with features that doesn't break all the crops of my sites, but its really difficult to understand everything that is going on under these module(for my, ofcourse).
please, some dev ops help needed here :)
Thanks!
Sebas.
Comment #8
sebas5384 CreditAttribution: sebas5384 commentedWell, the problem obviously is because of the missing ids, isid and ieid.
So I search and debug like hell and i found out that features.image.inc when is about to export the data of the image_style is removing those ids.
The thing is, if we apply the patch #2 all the previous cropped images won't work any more. And that is really, really, really bad when you have a site with more than 500 products, and other cropped images.
It seems that the only solution to this is a magical hook_update_N() to migrate the old crops.
And now the question is, whats the best way to do this?
We must do this before others sites appearing to have this problem to with features.
Cheers,
Sebas.
Comment #9
sebas5384 CreditAttribution: sebas5384 commentedHey! finally I made the update process to the new format without the isid thing.
So I mixup the patch #2 and the hook_update_N(), and now its all working like a charm!
Please give it a test and let me know ;)
The changes are all for the 7.x-2.2 version, so if this gets aproved probably we have to make a patch against the dev branch.
Cheers!,
Sebas.
Comment #10
yvmarques CreditAttribution: yvmarques commentedGreat, I will try it !
Thanks
Comment #11
nyl_auster CreditAttribution: nyl_auster commentedWorks like a charm :-)
Update works as expected; and now epsacrop work even if imagestyle are "default", exported by a feature.
Thanks sebas5384 !
Comment #12
nyl_auster CreditAttribution: nyl_auster commentedwell, seems does not work if there is no coords to update. I guess batch is incorrect : it should stop when progress / max = 1; but with no results, it seems that last division get stuck with "0 : -1" for ever and ever. (and as progress incrementation is in foreach, nothing happens when result array is empty).
I'll try to solve this tomorrow.
Comment #13
nyl_auster CreditAttribution: nyl_auster commentedTo solve infinite batch issue when there is nothing to update in database; i just add to the patch above :
I attached the full working patch.
Comment #14
cmalek CreditAttribution: cmalek commentedHas this branch been ported to the -dev branch yet?
Comment #15
cmalek CreditAttribution: cmalek commentedNevermind, http://drupal.org/files/crop_style_from_code-1396500-8.patch seems to apply cleanly to the latest -dev download.
Comment #16
JesseDP CreditAttribution: JesseDP commentedOverhere the update hook never dissapears.
I changed this line:
to
Comment #17
mattsmith3 CreditAttribution: mattsmith3 commented@ http://drupal.org/files/crop_style_from_code-1396500-8.patch
That patch had absolute paths in it. Here's another copy with out.
Here's another without those. Same patch...
Comment #18
mattsmith3 CreditAttribution: mattsmith3 commentedStill isn't working for me. It looks like the dialog is storing the crop areas but they aren't showing on display.
Edit: For existing installs, you have to disable and re-enable epsacrop. In my case, I had to re-add existing content too. Only new nodes worked.
Comment #19
olofbokedal CreditAttribution: olofbokedal commentedModified the patch from #17, which now removes both the effect id and style id from the naming convention.
If the image style exists in code, both the image style and its effects will be missing from the database, causing both style id and effect id to be NULL. I can't see a reason to keep the effect id, since there's no use case where you'd like to have multiple epsa crop effects for the same style?
I've modified the same update function from #17. It now makes sure that it could find a machine name for the image style, otherwise it won't get saved to the database. I've also removed the sandbox variable, since it caused the update function to stall.
Comment #20
ygerasimov CreditAttribution: ygerasimov commentedComment #21
dagomar CreditAttribution: dagomar commentedModified the patch in 19, I experienced an undefined index in case no cropping was selected.
Comment #22
nagy.balint CreditAttribution: nagy.balint commentedTested #21.
Seems to work fine.
Created a new patch without the whitespace problem, and applied for latest dev.
Would be nice to get this commited somehow, as on most sites the image styles are in features, which makes this a major issue indeed.
Comment #23
Aron NovakTested #22, works correctly and applies cleanly.
Comment #24
wesleydv CreditAttribution: wesleydv commented#22, works on latest dev, should be committed
Comment #25
yvmarques CreditAttribution: yvmarques commentedI have committed the patch #22, thanks for it.
Comment #26
yvmarques CreditAttribution: yvmarques commentedComment #28
coredumperror CreditAttribution: coredumperror commentedThe update function from this patch is both badly buggy and utterly terrifying, and should be uncommitted ASAP. Here's why:
1) The big problem is that db_delete() call. It's completely crazy to delete your users' crops if the conversion failed. Thank goodness that code path won't be followed if there were any coords set up for the image in question (because $new_coords won't be empty), or the crops for several thousand images on my site would have been erased with no way to recover them.
2) The code that changes old crop keys to the new format doesn't work on my site. I don't know why, but my site's old crop keys looked like "epsacrop--
", rather than the format of "epsacrop---" which the code appears to expect. That means that the conversion failed to convert my site's crop records. 3) Theif ($style['isid'] == $isid)
check causes a PHP error, because$style['isid']
doesn't exist. That problem stems from the difference in output format between the Drupal API functions image_styles() and image_style_load(). The first one returns all image style data in an associative array keyed by isid, while the second returns one image style's data with an 'isid' element in the output. This update function appears to expect the output fromimage_style_load()
, but what it actually gets is the output fromimage_styles()
, because that's what_epsacrop_load_styles()
calls. This means the check for$style['isid']
will never work, and will always throw an error. 4) It's explicitly stated in the docs fordrupal_write_record()
that it should never be called in an update hook, due to the schema not necessarily being in a solid state during the update process. That line should be a call todb_update()
instead. I wrote a custom script that does what this update hook was supposed to do, and I could adapt it to a replacement patch for EPSACrop if you like.Comment #29
yvmarques CreditAttribution: yvmarques commentedI would be grateful if you could share your work, I got great points and if we fix it soon as possible it would be great. Thanks.
Comment #30
coredumperror CreditAttribution: coredumperror commentedOK, here's a patch against the current dev version which adds a new update hook that uses the technique that my script uses. It also erases the code from the old, broken hook. Doing it this way (rather than just replacing the code in the broken hook), ensures that people who've already updated to the latest dev get the new hook when they update again and run the update script, and that people who haven't yet updated to the latest dev will not be given the broken code when they update now.
Comment #31
joelhsmith CreditAttribution: joelhsmith commentedOn my local copy of my site, I installed the dev version (2014-Mar-13) of EPSA Crop and applied #30. All my custom Image Styles got deleted. And all the images that relied on the Images Styles got resized. Its a huge site so there could be other issues at play.
So have a backup your sites before playing. :-)