Problem/Motivation

If you install Simple OAuth on Drupal 8.7 and you visit /admin/config/people/simple_oauth, you will get this exception:

PHP Fatal error: Class Drupal\simple_oauth\Service\Filesystem\Filesystem contains 8 abstract methods and must therefore be declared abstract or implement the remaining methods (Drupal\Core\File\FileSystemInterface::copy, Drupal\Core\File\FileSystemInterface::delete, Drupal\Core\File\FileSystemInterface::deleteRecursive, ...) in modules/simple_oauth/src/Service/Filesystem/Filesystem.php on line 10

Methods were added to FileSystemInterface in Drupal 8.7, apparently, so this module is unusable until all the new methods are implemented. :(

Proposed resolution

Implement the missing methods.

Remaining tasks

Patch, review, commit.

CommentFileSizeAuthor
#14 3043454--interdiff--11-14.txt3.37 KBe0ipso
#14 3043454--filesystem-interface-changes--14.patch14.21 KBe0ipso
#14 3043454--filesystem-interface-changes--14.patch14.21 KBe0ipso
#13 3043454--filesystem-interface-changes--11.patch10.84 KBe0ipso
#6 3043454--filesystem-interface-changes--6.patch7.86 KBe0ipso
#5 3043454--filesystem-interface-changes--5.patch6.21 KBe0ipso
#3 3043454-3.patch2.58 KBphenaproxima
#2 3043454-2.patch1.74 KBphenaproxima

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
StatusFileSize
new 3043454-2.patch1.74 KB

This fixes it for me. Not sure if this needs tests, but happy to add one if asked.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 3043454-3.patch2.58 KB

Added a unit test. Confirmed locally that removing the new methods reproduces the correct failure.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Category: Bug report » Task
Issue tags: +API-First Initiative

LGTM. And AFAICT this will work fine on Drupal 8.5 and 8.6 too: they don't have these methods, but there this can simply be considered "dead code that will come alive in the future".

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
StatusFileSize
new 3043454--filesystem-interface-changes--5.patch6.21 KB

Let's see if this patch mitigates the risk of breaking this in the future. The current implementation is not ideal, we should break the 2 services… but this is a good step forward.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
StatusFileSize
new 3043454--filesystem-interface-changes--6.patch7.86 KB

I'll merge this one if green, it should be more resilient than the existing code and the previous patches in this issue.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
+++ b/src/Service/Filesystem/Filesystem.php
@@ -66,80 +66,20 @@ class Filesystem implements FilesystemInterface {
+  public function __call($name, $arguments) {

Why didn't I think of this? 😄

Curious what @Berdir thinks of this :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

That only works if you stop implementing the interface. And then it IMHO would not be possible to inject that service into something that type hints on FileSystemInterface?

I think a separate service that does not decorate is nicer here anyway, we only want to do something special on our specific use cases (I actually don't know what that is), it seems strange that a module like simple_oauth intercepts everyone's interaction with the file system service.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#8: hah, that's why I didn't think of that. Why didn't I think of that? (i.e. Berdir's response) 😄

Seems like I'm multi-tasking too much and missing important details.

it seems strange that a module like simple_oauth intercepts everyone's interaction with the file system service.

This is most definitely true.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Looks like it needs a reroll?

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Let's see if this one applies.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs work » Needs review
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
StatusFileSize
new 3043454--filesystem-interface-changes--11.patch10.84 KB

The actual patch 🤦‍♀️

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
StatusFileSize
new 3043454--filesystem-interface-changes--14.patch14.21 KB
new 3043454--filesystem-interface-changes--14.patch14.21 KB
new 3043454--interdiff--11-14.txt3.37 KB

Some additional fixes.

  • e0ipso committed c1bf3d7 on 8.x-3.x 
    Issue #3043454 by e0ipso, phenaproxima, Wim Leers, Berdir: Fatal error...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Fixed

Thanks everyone!

Status: Fixed » Closed (fixed)

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

hiddenfellon’s picture

hiddenfellon commented

This actually needs to be reopened. I applied your patch and now I get this error Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "simple_oauth.filesystem_checker". Did you mean this: "simple_oauth.filesystem"? in Drupal\Component\DependencyInjection\Container-
>get() (line 153 of /public_html/core/lib/Drupal/Component/DependencyInjection/Container.php).

not to mention I still change access the config settings to setup a client.

shahankitb1982’s picture

shahankitb1982 commented

Hi @hiddenfellon. Just clear the cache, then it should be fine.