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.
Focal point preview is not rendered when a original image is not public or private schema, such as s3. This case, focal point preview page is displaying broken link for each styles image.
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 6.69 KB | bleen |
#22 | focal_point_preview_is_not_rendered-2824779-22.patch | 5.31 KB | bleen |
| |||
#17 | focal_point_preview_is_not_rendered-2824779-17.patch | 3.21 KB | cyslee |
|
Comments
Comment #2
cyslee CreditAttribution: cyslee at NBCUniversal commentedComment #3
cyslee CreditAttribution: cyslee at NBCUniversal commentedfocal_point_preview_is_not_rendered-2824779-3.patch is for fixing git hash, 3095c52246e3a45c708649d07dafe0505acb0659 version
Comment #5
cyslee CreditAttribution: cyslee at NBCUniversal commentedFor git hash 3095c52246e3a45c708649d07dafe0505acb0659, fixing focal point preview in s3 patch, please use patch focal_point_preview_is_not_rendered-2824779-4.patch
Comment #6
bleen CreditAttribution: bleen at NBCUniversal commentedhere testbot :)
Comment #8
bleen CreditAttribution: bleen at NBCUniversal commented... not sure why the patch isn't applying.
Is this needed for buildUrl to work or just for setting the dimensions down below. If its just for the dimensions down below I think it can go away (see my other comments). If its needed by buildUrl() then we need to talk :)
I think this can just be
I don't think the rest is necessary.
Doesn't $variables['image'] already exist? cant we get rid of all of this and just put:
$variables['image']['#uri'] => $style_url
And if so, then see my first note above...
Comment #9
cyslee CreditAttribution: cyslee at NBCUniversal commented@bleen 3095c52246e-focal_point_preview_is_not_rendered-2824779-9.patch is for focal_point version, 3095c52246e3a45c708649d07dafe0505acb0659 one. Also I cleaned up existing patch. focal_point_preview_is_not_rendered-2824779-9.patch is only update
$variables['image']['#uri']
value.Comment #10
bleen CreditAttribution: bleen at NBCUniversal commentedsetting to "needs review" so testbot will come back
Comment #11
bleen CreditAttribution: bleen at NBCUniversal commentedThis looks much better...
Comment #12
jfrederick CreditAttribution: jfrederick commentedIn these lines:
$style
can be returned as NULL. Should check that before moving on.In these lines:
is
strstr()
returns 0, it found the string, but then the conditional will fail. Should we be checkingif($style_path !== NULL)
?Comment #13
cyslee CreditAttribution: cyslee at NBCUniversal commentedPrevious patch didn't work, so I created new one. new version is downloading image from s3 under local focal_point_preview directory. Therefore, when a preview page is loading, it is using the local downloaded image to generate image styles in preview page. This downloaded image will be deleted from file system and file entity list by "Delete Orphaned file" settings in file system settings page, /admin/config/media/file-system
Comment #14
bleen CreditAttribution: bleen at NBCUniversal commentedThis looks pretty good. Most of my comments are pretty minor...
This needs a "why" comment
I think this should just be called "getOriginalImage" the fact that we make sure it is local can be explained in comments
I don't know that I would include a message here.
Comment #15
cyslee CreditAttribution: cyslee at NBCUniversal commented@bleen, I updated patch by your comment review. Please check again.
Comment #16
cyslee CreditAttribution: cyslee at NBCUniversal commentedfound out focal_point_preview_is_not_rendered-2824779-15.patch is incorrect, so uploaded the correct version.
Comment #17
cyslee CreditAttribution: cyslee at NBCUniversal commentedI found out one problem in focal_point_preview_is_not_rendered-2824779-16.patch. The updated focal point is not render correct in preview page because
getOriginalImage()
is called beforeimage_path_flush($image->getSource());
. focal_point_preview_is_not_rendered-2824779-17.patch is for fixing the loading correct focal point preview page.Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedWhats the best S3 module? Would be useful for testing this.
Comment #19
bleen CreditAttribution: bleen at NBCUniversal commented"Best" is kindof a strong term ... we are using https://www.drupal.org/project/flysystem
Comment #20
bleen CreditAttribution: bleen at NBCUniversal commentedComment #21
a.milkovskyWorks for me.
Using https://www.drupal.org/project/flysystem_s3
Comment #22
bleen CreditAttribution: bleen at NBCUniversal commentedThis version of the patch fixes some dependency injection, some grammar and a few other minor things....
That said, I'm concerned about committing this because, in the best case, there is a period of time (in between when preview images are generated and when cron deletes orphaned files) when image entities are duplicated on the site. This may cause issues for reference field, with duplicates showing up in the media library, etc...
Is there another way to solve this? Is there a way to hide all the local images from the rest of the application? Something else?