craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.27k stars 635 forks source link

EVENT_GENERATE_TRANSFORM old $asset Data. #2285

Closed nielsnuebel closed 6 years ago

nielsnuebel commented 6 years ago

Description

just for my understanding: I change an Asset with a new FocalPoint and i want to create a different set of Images with the new FocalPoint value. I have to use generateTransform Trigger, right?

The Problem is that the trigger contains old Values from Database and not the new Data.

  1. The Controller get the new Values https://github.com/craftcms/cms/blob/develop/src/controllers/AssetsController.php#L722

  2. But AssetTransform get the old Values from Database before the new Values are saved. https://github.com/craftcms/cms/blob/develop/src/services/AssetTransforms.php#L566

call mehtod with the old values here https://github.com/craftcms/cms/blob/develop/src/services/AssetTransforms.php#L618

  1. add trigger EVENT_GENERATE_TRANSFORM https://github.com/craftcms/cms/blob/develop/src/services/AssetTransforms.php#L1362

Steps to reproduce

  1. Upload a Image and add FocalPoint -> save
  2. open again the image and Change the FocalPoint

Additional info

andris-sevcenko commented 6 years ago

I'm not sure I follow. If you use the Image Editor, change the focal point and replace the existing image, you trigger this code https://github.com/craftcms/cms/blob/develop/src/controllers/AssetsController.php#L741-L755 which deletes all created transforms for the Asset and saves the Asset.

No new transforms are being created here, so generateTransform is not triggered at this point at all.

The event is being triggered only when rendering a page with transforms in it, at which point the event is triggered, and the new values are fetched.

Am I missing something here?

nielsnuebel commented 6 years ago

craftcms

You're right, the Plugin craft3-imageoptimize trigger this event before craft saved the asset.

andris-sevcenko commented 6 years ago

@nielsnuebel can you show the full stack that triggers the event? Specifically, I want to know what event triggers the transform chain.

nielsnuebel commented 6 years ago

@khalwat extend the field with the method normalizeFieldValue https://github.com/nystudio107/craft3-imageoptimize/blob/master/src/fields/OptimizedImages.php#L175

after four method here is the getAssetUrl method https://github.com/nystudio107/craft3-imageoptimize/blob/master/src/imagetransforms/CraftImageTransform.php#L46

image

andris-sevcenko commented 6 years ago

Right, so I'd say that Andrew is attaching to the wrong event - he's re-generating the optimized images before the Asset is saved to the database and ends up using the old focal point values.

For now, I'll throw it back to him and we'll see if we can work something out.

khalwat commented 6 years ago

Andrew is having a look. Related: https://github.com/nystudio107/craft3-imageoptimize/issues/29

khalwat commented 6 years ago

So @nielsnuebel are you indeed running Craft CMS RC1? If so, you should update to RC5 (the latest); @andris-sevcenko rolled in some fixes that take care of this neatly.

khalwat commented 6 years ago

Alright so what's happening here is when an Asset is saved, my Field's normalizeValue() gets called, which tries to create all of the transforms needed so they can be saved into the model that ends up getting saved as my Field's value.

The problem is that the new focal point hasn't yet been saved, so it's re-using the old Focal Point when it generates the images. If you then just try to re-save the Asset without moving the Focal Point, the image editor properly recognizes that nothing has changed, and it doesn't nuke the asset transforms.

If you move the Focal Point a little bit, the resave happens, and my plugin will regenerate the responsive images, but it will be using the Focal Point from the previous save.

I'm not sure at this point how to handle it, because the Asset rightly wants the data from any of its fields before trying to save itself, but I can't generate the image transforms using the new Focal Point, because it's not saved in the Asset yet.

So it's a bit of a chicken and egg situation. I'll look for a reasonable solution to this.

khalwat commented 6 years ago

The other thing that I consider a little weird about all this is that in my simplified test bed, everything actually does work properly. Really strange.

khalwat commented 6 years ago

TL;DR:

1) I want to store responsive image variant URLs in my Field’s data, which is attached to an Asset via the Asset Volume’s Field Layout. This generates the transforms on Asset save rather than on page load, which is what we want for performance reasons.

2) The right time to save this Field data is when the Asset is being saved; it’s part of the data associated with the Asset

3) If I try to do the above, the new Focal Point hasn’t been saved to the db yet, which causes the Craft routines that end up getting called to use the old (stale) Focal Point

khalwat commented 6 years ago

@brandonkelly and @andris-sevcenko helped me figure out a better way to do this; this can be closed.