Support from Acquia helps fund testing for Drupal Acquia logo

Comments

malte.koelle created an issue. See original summary.

malte.koelle’s picture

I have removed the two deprecated functions with the suggested functions from Upgrade Status Module.

I hope this works.

malte.koelle’s picture

Status: Active » Needs review
malte.koelle’s picture

FileSize
1.87 KB
1006 bytes

I removed the @ Sign and replaced it with a try catch clause.

andypost’s picture

+++ b/src/Controller/ImageStyleDownloadController.php
@@ -112,7 +112,7 @@ class ImageStyleDownloadController extends FileDownloadController {
+    $valid = !empty($image_style) && \Drupal::service('file_system')->validScheme($scheme);

+++ b/src/Webp.php
@@ -127,7 +128,11 @@ class Webp {
+      \Drupal::service('file_system')->deleteRecursive(file_default_scheme() . '://styles');

Better to use DI for the services

malte.koelle’s picture

Thank you @andypost for your answer.
I have changed my code and I'm using DI now for the services.

dermario’s picture

Status: Needs review » Needs work

The DI implementation looks fine from my side. I found some minor stuff regarding code style.

+++ b/src/Controller/ImageStyleDownloadController.php
@@ -58,12 +66,14 @@ class ImageStyleDownloadController extends FileDownloadController {
+   * @param \Drupal\Core\File\FileSystem $fileSystem

The description of $fileSystem missing.

I'm not sure whether it's "wrong" to use lowerCamelCase variable names here. To stay consistent with $image_factory we maybe could use $file_system here.

malte.koelle’s picture

Thank you @dermario for your fast response

I have changed the variable name to a lowerCamelCase name.

malte.koelle’s picture

I changed the variable name from fileSystem to file_system for the Webp.php class as well.

malte.koelle’s picture

Status: Needs review » Needs work

I realised that my patch isn't working. I will have a look on it.

malte.koelle’s picture

I added the file system service as an argument in the services.yml file. Now it works after testing it.

alexmoreno’s picture

thanks all, looks good to me.

alexmoreno’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

bjaxelsen’s picture

Current development version does not work on Drupal 9 - I've created patch.