When using Media Gallery with the media module and s3fs images are not being displayed. Instead all images were just displayed at file links.

This was due to no image styles being applied when rendering because file_entity_file_is_local() returns FALSE.

The function file_entity_file_is_local() checks the available file systems which advertise as local STREAM_WRAPPERS_LOCAL. I think this is to specific as the functionality does work nicely with the s3fs module and so broadening this criteria to read and write with STREAM_WRAPPERS_WRITE_VISIBLE will be more specific.

CommentFileSizeAuthor
#12 file-entity-file-is-readable-2550231-12.patch1.15 KBJoeMcGuire
#2 STREAM-WRAPPERS-WRITE-VISIBLE-2550231-2.patch571 bytesJoeMcGuire
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JoeMcGuire created an issue. See original summary.

JoeMcGuire’s picture

JoeMcGuire CreditAttribution: JoeMcGuire commented
Status: Active » Needs review
FileSize
STREAM-WRAPPERS-WRITE-VISIBLE-2550231-2.patch571 bytes

Proposed solution attached to detect stream wrappers on the more specific read and write flags instead of local using STREAM_WRAPPERS_WRITE_VISIBLE

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Needs work

This changes the API, and cannot be changed in this way. We should change file_entity_file_formatter_file_image_view() instead if needed.

JoeMcGuire’s picture

JoeMcGuire CreditAttribution: JoeMcGuire commented

Hi Dave,

Firstly thanks for all your great work on Drupal, I see your name around a lot.

Can you give a bit more insight into which API you are concerned about being broken?

We did review removing the check all together from file_entity_file_formatter_file_image_view() but concluded that read only file systems would fail ungracefully.

The module appears to work very nicely with s3fs once this patch is applied. s3fs describes itself as read and write, but critically NOT being local; which I think is descriptive.

This approach (in theory) just alters the demands on the file system wrapper so that any which can read and write can use the functionality regardless of being local or not.

Many thanks,

Joe

coredumperror’s picture

coredumperror CreditAttribution: coredumperror commented

The reason your fix isn't viable is that you're changing what a call to file_entity_file_is_local() means. It no longer returns what you'd expect based on the name of the function. The right solution would probably be to replace any relevant calls to file_entity_file_is_local() with calls to a new function that does what your patched version does, e.g. file_entity_file_is_readwrite().

JoeMcGuire’s picture

JoeMcGuire CreditAttribution: JoeMcGuire commented

Thanks coredumperror.

That sounds reasonable and I can certainly follow up tomorrow with a more thorough patch as you have described.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

file_entity_file_is_local() is an API function that determines if the file is local or not. Changing the behavior of that API function would mean we would have to rename it to file_entity_file_is_writeable() or something else, because keeping the name the same would not match what it actually does.

JoeMcGuire’s picture

JoeMcGuire CreditAttribution: JoeMcGuire commented

Thanks Dave for such a prompt response

I'll follow up tomorrow with a more thorough patch inline with you are coredumperrors direction.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

FYI this was fixed in File Entity 7.x-2.x with the following issue: #1358860: File Entity won't render image derivatives for images stored using non-local stream wrappers so I'd probably recommend the same fix.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Or alternatively taking a look at the current file_entity_file_formatter_file_image_view() function in File Entity 7.x-2.x and copying the changes we need.

JoeMcGuire’s picture

JoeMcGuire CreditAttribution: JoeMcGuire commented

Thanks Dave. I'll aim to mimic to keep consistency.

JoeMcGuire’s picture

JoeMcGuire CreditAttribution: JoeMcGuire commented
FileSize
file-entity-file-is-readable-2550231-12.patch1.15 KB

Thanks Dave and Coredumperror.

Attached is an updated patch which implements file_entity_file_is_readable() inline with the 2.x branch fix at #1358860: File Entity won't render image derivatives for images stored using non-local stream wrappers

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews as a volunteer commented
Status: Needs work » Closed (outdated)

Closing this issue as outdated. However, if you think this issue is still important, please let us know and we will gladly re-open it for review.
sincerely,
- the Drupal Media Team