Problem/Motivation

core/modules/media/src/Plugin/media/Source/Image.php is using Drupal\Core\File\FileSystem class instead of Drupal\Core\File\FileSystemInterface for type hinting in its constructor.
This causes problem when someone wants to use custom FileSystem class instead of default one.
It works fine if we use FileSystemInterface instead of FileSystem.

CommentFileSizeAuthor
#16 2973509-d85.patch2.47 KBdave reid
#5 2973509-5.interdiff.txt3.23 KBclaudiu.cristea
#5 2973509-5.patch5.24 KBclaudiu.cristea
#2 2973509-1.patch2.01 KBtahirmus

Comments

tahirmus created an issue. See original summary.

tahirmus’s picture

tahirmus commented
StatusFileSize
new 2973509-1.patch2.01 KB

Replaced type hinting from FileSystem to FileSystemInterface

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Active » Reviewed & tested by the community

This looks good to me, but I am not 100% sure if this is BC breaking. But I don't think so.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

This looks good but in order to swap out the FileSystem we need to make sure this is the case everywhere - so we need to fix \Drupal\file\Plugin\rest\resource\FileUploadResource too otherwise the aims of this issue can't be achieved. That and the one already fixed here are the only two incorrect typehints on FileSystem instead of FileSystemInterface I can find in core.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review
StatusFileSize
new 2973509-5.patch5.24 KB
new 2973509-5.interdiff.txt3.23 KB

And another one in a test but only in docs.

Status: Needs review » Needs work

The last submitted patch, 5: 2973509-5.patch, failed testing. View results

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Version: 8.5.x-dev » 8.6.x-dev
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review
chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

Nice, back to RTBC

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 44986fe and pushed to 8.6.x. Thanks!

  • catch committed 44986fe on 8.6.x 
    Issue #2973509 by claudiu.cristea, tahirmus, alexpott: Image media...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Closed (fixed) » Patch (to be ported)

Any reason this shouldn't be backported to 8.5.x? This is a bug for anyone using the Flysystem module and core media.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.5.x-dev » 8.6.x-dev
Status: Patch (to be ported) » Fixed

@Dave Reid - yep there is a reason. 8.5.x is officially closed to bug fixes and only gets security support. Also closed issues are only supposed to be re-opened by maintainers.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

My apologies for asking and for re-opening the ticket. I'm still a bit confused about the timeline because at the time when this was fixed in 8.6.x, 8.6.0 hadn't been released yet, so 8.5.x was the currently supported branch. I've had two different clients run into this one in the past two months. Seems not much can be done now?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 2973509-d85.patch2.47 KB

Here is a patch rolled against 8.5.x if anyone needs it.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.