Image effect convert fails when image file is in the public files root

This only occurs if the source image is in the root file folder

So this is an edge case, right? I think this needs to be reflected in the IS, and we definitely need a test to show the bug.

I don't know if this can truly be considered an edge case. There are plenty of sites that will drop a lot of their file uploads directly into the public file folder instead of organizing them into subfolders. The issue is that the default PHP function pathinfo strips the trailing slashes from the "dirname" which breaks any images being converted to a different image type that are stored in the root public folder.

This is still an issue, i'm rerolling this patch against 8.5. Still needs tests, but for the record, files in the root of the public folder isn't really an edge case, there are lots of reasons they may be there. My particular case came from a d7-d8 migration, where the files were copied over in the root because that's where they were in D7

Here's a test.

Fixed the PHPCS nit and updated issue title and summary.

1. +++ b/core/modules/image/tests/src/Functional/ImageEffect/ConvertTest.php
   @@ -0,0 +1,54 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   Can be replaced by {@inheritdoc}

2. +++ b/core/modules/image/tests/src/Functional/ImageEffect/ConvertTest.php
   @@ -0,0 +1,54 @@
   +  public static $modules = ['image', 'simpletest'];
   

   Why does this need the simpletest module?

#17.2 because there's a drupal_get_path('module', 'simpletest') a few lines below. TBH I do not know if that needs the module to be installed though. If it doesnt than yep that's supefluous.

Did changes as per #17 and #18 , please review it :)

#17.2 wasn't fixed. But the test passes with the simpletest module removed from the modules to install.

I am at the Drupalcon Nashville 2018 mentored sprint. I'm going to review this issue now.

Unable to reproduce. I receive a 403, not a 404.

The instructions to reproduce are unclear (to me): "call the style url". I don't know what this means. My best interpretation was to hit the direct URL path where the "styled" image should reside. To test this, I created my image style (test_image_style) and changed the Article content type image field to use it for display. I then created an article, added an image, viewed the node, right clicked the image, and opened it in a new tab, which had this URL:

http://sprint-20180413-0916.ddev.local:8080/sites/default/files/styles/t...

I placed a new file, test.png, directly in the /sites/default/files directory, then attempted to access it using these modified URLs:

http://sprint-20180413-0916.ddev.local:8080/sites/default/files/styles/t...
http://sprint-20180413-0916.ddev.local:8080/sites/default/files/styles/t...
http://sprint-20180413-0916.ddev.local:8080/sites/default/files/styles/t...

All resulted in a 403, but I have confirmed that no image files exist at /sites/default/files/styles/test_image_style/public (so, why am I NOT getting a 404?).

Regardless, for kicks, I applied the patch. No change in the above-described behavior.

I assume I'm missing something. I'd be happy to re-test if someone can point out what I'm doing wrong.

Your 403 comes from the fact that you haven't generated an image token to append to the end to authorize the image style generation. This one is tricky to reproduce because D8 doesn't want to put images in the root of the public files directory.

I haven't tested this, but off the top of my head, to reproduce without custom code, go with your first method, but edit your image field settings and remove [date:custom:Y]-[date:custom:m] from the File directory setting, so that the image is uploaded directly to the public files directory. Then you should get your proper tokenized url where the system cannot convert the file.

Confirming that this bug still exists in 8.6 and 8.7, and the patch from #20 is still valid and solves the issue. I'm rerunning tests against 8.6 and 8.7, but I touched the patch above, so I can't RTBC.

This does not work if your default file scheme is private and you are trying to generate an image style for a public file.

file_build_uri ends up calling file_default_scheme. which is not what we want.

Working on this

Not sure if we are allowed to change constructors, but would be nice to actually use a service instead of file_uri_scheme or static container calls.

Lets see if the tests pass at least.

A bit quick, my patch was not in patch format :D

We can change the constructor AFAIK, since controllers use dependency injection. The signature of the create method is unchanged.

It probably needs an additional test testing against your case of a default private scheme and generating a style for a public file.

Yes i know but if my custom module extends this class my code would break. So it might be a bc breaking change.

Agree with the test. Just wanted to see if the tests passed first :)

While that is true, controller constructors are not considered part of the API, and are subject to change, so you are all good.

@see https://www.drupal.org/core/d8-bc-policy#constructors

Patch rerolled

I Sorry patch #32 at comment #33 (first bad) is for 8.8.x version but I note that for this 8.8.x version the issue does not happen

I have tested 8.8.1 without the patch and this is still an issue. The reroll from comment #33 has a missing comma and a syntax error, so I have fixed that missing comma and the patch fixes the issue.

Here is the patch for version 8.8.x and the interdiff against the one from comment #33

Reroll to fix the unit tests and remove some deprecations.

We're using this patch with Drupal 8.8.2 in production with success so far. Without this patch png images in the root files folder could not be generated as image styles, with it they work as expected.

Edit: Patch from #37 that is.

Also encountered this issue in 8.8.5 after the adoption of responsive image and a series of image styles. Patch #37 works.

Also encountered this issue in 8.9.3. Patch #37 works beautifully.

1. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -51,12 +60,15 @@ class ImageStyleDownloadController extends FileDownloadController {
   -  public function __construct(LockBackendInterface $lock, ImageFactory $image_factory, StreamWrapperManagerInterface $stream_wrapper_manager = NULL) {
   +  public function __construct(LockBackendInterface $lock, ImageFactory $image_factory, StreamWrapperManagerInterface $stream_wrapper_manager = NULL, FileSystemInterface $file_system = NULL) {
        parent::__construct($stream_wrapper_manager);
        $this->lock = $lock;
        $this->imageFactory = $image_factory;
        $this->logger = $this->getLogger('image');
   +    $this->fileSystem = $file_system;
      }
   

   I think we need to make the assignment conditional with deprecation - if $file_system is null, it should be got form \Drupal::service

2. +++ b/core/modules/image/tests/src/Functional/ImageEffect/ConvertTest.php
   @@ -0,0 +1,58 @@
   +  public static $modules = ['image'];
   

   protected

3. +++ b/core/modules/image/tests/src/Functional/ImageEffect/ConvertTest.php
   @@ -0,0 +1,58 @@
   +    $this->assertEqual(SAVED_NEW, $image_style->save());
   ...
   +    $this->assertEqual(SAVED_UPDATED, $image_style->save());
   

   assertEquals

4. +++ b/core/modules/image/tests/src/Functional/ImageEffect/ConvertTest.php
   @@ -0,0 +1,58 @@
   +    $this->assertResponse(200);
   

   $this->assertSession()->statusCodeEquals(200);

Made the changes requested in #44. Please review.

Thanks @ayushmishra206!

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -51,12 +60,18 @@ class ImageStyleDownloadController extends FileDownloadController {
-  public function __construct(LockBackendInterface $lock, ImageFactory $image_factory, StreamWrapperManagerInterface $stream_wrapper_manager = NULL) {
+  public function __construct(LockBackendInterface $lock, ImageFactory $image_factory, StreamWrapperManagerInterface $stream_wrapper_manager = NULL, FileSystemInterface $file_system) {

we cannot have a mandatory argument ($file_system) after an optional one ($stream_wrapper_manager), so $file_system = NULL is correct.

Re #44.1, we should have something like this:

  /**
   * Constructs a ArchiverManager object.
   *
   * @param \Traversable $namespaces
   *   An object that implements \Traversable which contains the root paths
   *   keyed by the corresponding namespace to look for plugin implementations.
   * @param \Drupal\Core\Cache\CacheBackendInterface $cache_backend
   *   Cache backend instance to use.
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   *   The module handler to invoke the alter hook with.
   * @param \Drupal\Core\File\FileSystemInterface $file_system
   *   The file handler.
   */
  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, FileSystemInterface $file_system = NULL) {
    parent::__construct('Plugin/Archiver', $namespaces, $module_handler, 'Drupal\Core\Archiver\ArchiverInterface', 'Drupal\Core\Archiver\Annotation\Archiver');
    $this->alterInfo('archiver_info');
    $this->setCacheBackend($cache_backend, 'archiver_info_plugins');
    if (!isset($file_system)) {
      @trigger_error('Not defining the final $file_system argument to ' . __METHOD__ . ' is deprecated in drupal:8.8.3 and will throw an error in drupal:10.0.0.', E_USER_DEPRECATED);
      $file_system = \Drupal::service('file_system');
    }
    $this->fileSystem = $file_system;
  }

with a @trigger_error if $file_system is null.

@ayushmishra206 thanks for the work. I updated the patch as suggested by @mondrake. Please review.

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -51,12 +60,20 @@ class ImageStyleDownloadController extends FileDownloadController {
+      @trigger_error('Not defining the final $file_system argument to ' . __METHOD__ . ' is deprecated in drupal:8.8.3 and will throw an error in drupal:10.0.0.', E_USER_DEPRECATED);

deprecated in drupal:9.1.0

Updated the @trigger_error message by replacing drupal:8.8.3 to drupal:9.1.0.

Thank you, RTBC - I shouldn't really but #38, #40 and #42 are supporting evidence.

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -51,12 +60,20 @@ class ImageStyleDownloadController extends FileDownloadController {
+      @trigger_error('Not defining the final $file_system argument to ' . __METHOD__ . ' is deprecated in drupal:9.1.0 and will throw an error in drupal:10.0.0.', E_USER_DEPRECATED);

Nit: what is the use of final here for - we don't normally reference the argument position right?

Also - shouldn't we be adding a new deprecation test for this - per item 3 of https://www.drupal.org/core/deprecation#how

Per #3170185-14: Drupal\taxonomy\Form\OverviewTerms should determine the current page via the 'pager.manager' service, not from the Request, the deprecation test seems not necessary in this case.

NW to remove the word "final" from

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -51,12 +60,20 @@ class ImageStyleDownloadController extends FileDownloadController {
+      @trigger_error('Not defining the final $file_system argument to ' . __METHOD__ . ' is deprecated in drupal:9.1.0 and will throw an error in drupal:10.0.0.', E_USER_DEPRECATED);

Working on #54

Made the change requested in #54. Please review.

Thanks!!

Random fail in #58

We need a follow-up to fix StreamWrapperManagerInterface $stream_wrapper_manager = NULL - that cannot be NULL - we missed this in #3097453: Remove system.module BC layers

Committed aa12b0e and pushed to 9.1.x. Thanks!

Filed #3174913: Fix StreamWrapperManagerInterface $stream_wrapper_manager argument in the constructor of ImageStyleDownloadController.

What about fixing 8.x versions which have this annoying problem too?

Patch applies to 8.9.x as well, but here it is. It's not possible for me to re-open this, but let's discuss if we should fix it first?

Also, the patch includes a text about deprecating in 9.1.x. Should we leave that?