\Drupal calls should be avoided in classes, use dependency injection instead

Kindly review patch.

The last submitted patch, 2: 3371019-2.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Thanks for posting this Arti ...

I have posted a separate issue relating to JuiceboxFormatter.php and dependency injection. There are also some separate pending patches waiting review for the 4.0 release that may have some effect on testing. But I have all the patched "stuff" working on a local Wampserver installation and I'll try your patch there within the next few days. I want to update to 10.1 first. Then I'll run your patched code through PHPCS to see if the dependency injection error is fixed, then I'll try it on Wampserver and then finally on my hosted production site.

This may take a few days because of time constraints. Locally using PHPStorm all the Juicebox 4.0 code passes PHPCS with the exception of JuiceboxFormatter as the error message you posted indicates.

I know that progress appears slow but we'll get there this Summer.

Update @arti ...

I applied your patch to JuiceboxFormatter manually to my copy that's kept in PHPStorm and used in local testing. I ran it through PHPCS with both the --standard=Drupal and the --standard=DrupalPractice. With --standard=Drupal phpcbf fixed up some typos I made in applying your patch. --Standard=DrupalPractice was the option that was pointing the finger at Dependency injection errors. the good news is that your patch seems to make the injection errors go away. Yeah!

I have also updated my local and shared sites to 10.1. So I need to do more actual testing ... I have lots of Photo Albums to test as well as some other Juicebox functions run through views. I want to do that first and take a few days. Assuming that goes well I think I'll be able to give it a RTBC and see how Luke wants to handle the commits.

We have a fair amount of "stuff" to push through and we'll have to decide how to handle them in terms of versioning.

Thank your for your contribution. !!

Looking at the test results and analyzing the code I wonder:

the patch does this:

 /**
  * Class to define a Drupal service with common formatter methods.
@@ -79,6 +80,20 @@ class JuiceboxFormatter implements JuiceboxFormatterInterface, TrustedCallbackIn
    */
   protected $entityTypeManager;
 
+  /**
+   * The file URL generator.
+   *
+   * @var \Drupal\Core\File\FileUrlGeneratorInterface
+   */
+  protected $fileUrlGenerator;
+
+  /**
+   * The module handler interface service.
+   *
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface
+   */
+  protected $moduleHandler;
+

The errors in the test results seem to indicate that we might need:

 /**
   * A Drupal entity type manager service.
   *
   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   */
    protected EntityTypeManagerInterface $entityTypeManager;

  /**
   * The file URL generator.
   *
   * @var \Drupal\Core\File\FileUrlGeneratorInterface
   */
    protected FileUrlGeneratorInterface $fileUrlGenerator;
  /**
   * The module handler interface service.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   */
  protected ModuleHandlerInterface $moduleHandler;

I'm out of my depth on some of this dependency injection stuff, but it looks to me like where the tests are failing with: TypeError: Drupal\juicebox\JuiceboxFormatter::__construct(): Argument #9 ($file_url_generator) must be of type Drupal\Core\File\FileUrlGeneratorInterface, Drupal\Core\Entity\EntityDisplayRepository given, called in /var/www/html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259

It could be because of the way the protected variables are coded. ???

Still PHPCS seems satisfied but the test results make me raise this question.

@arti:

In #6 I misread your patch. After beating my head against PHPCS and several versions of the code I can see that what you've proposed does include the proper "FileUrlGeneratorInterface $file_url_generator,
and ModuleHandlerInterface $module_handler"

and with that code it gets a pass on dependency injection from PHPCS --standard=DrupalPractice.

I think we should be good to go. I want to do a little more "real life" testing by moving this code to my hosted live system and see if anything breaks. Then we should be good for a rtbc. There are some other pending patches that should probably be moved live as part of an 4. something release. Maybe even a beta. That will be up to Luke.

Re. the test failures.

The tests fail in the juicebox/tests/src/Functional/ directory. All the programs there are still using the deprecated dependency injection methods. For instance, JuiceboxViewsCase has:

$this->assertSession()->responseContains(Html::escape(\Drupal::service('file_url_generator')->generateString($test_image_url)));

Which resembles the "old" code in Juiceboxformatter and not the "new" code in the patched Juiceboxformatter. The new code would be, for instance:

$image_data['unstyled_src'] = $this->fileUrlGenerator->generateAbsoluteString($file->getFileUri());

So, I think we might also need to patch the test files to reflect the dependency injection changes in JuiceboxFormatter. ??? Or just temporarily turn off running the functional tests (using the drupalci.yml file? Get JuiceboxFormatter patched, then the rest of the pending patches and come back and adjust the test files later. ??? This is getting into the territory where Luke is expert and I am NOT. Here is one of the errors displayed on the test console:

1) Drupal\Tests\juicebox\Functional\JuiceboxViewsCase::testViewsAdvanced
TypeError: Drupal\juicebox\JuiceboxFormatter::__construct(): Argument #9 ($file_url_generator) must be of type Drupal\Core\File\FileUrlGeneratorInterface, Drupal\Core\Entity\EntityDisplayRepository give

I'm not even sure if the test code is running against the patched version of JuiceboxFormatter or not. But clearly we have not patched the test directory to pick up the corrected dependency injection approach.

The type error that I listed in #6 still exists on a patched version of JuiceboxFormatter. The problem is not, or not exclusively in the /test directory files, the patched version of JuiceboxFormatter fails. You can reproduce this by trying to run admin/config on a test site. You will get an "unexpected error" message and in Reports/Recent Log Messages you will see:

TypeError: Drupal\juicebox\JuiceboxFormatter::__construct(): Argument #9 ($file_url_generator) must be of type Drupal\Core\File\FileUrlGeneratorInterface, Drupal\Core\Entity\EntityDisplayRepository given, called in D:\webpage\compg\web\core\lib\Drupal\Component\DependencyInjection\Container.php on line 259 in Drupal\juicebox\JuiceboxFormatter->__construct() (line 121 of D:\webpage\compg\web\modules\contrib\juicebox\src\JuiceboxFormatter.php).

I am trying to track down what is causing this.

The patch does make the PHPCS dependency injection issue go away, so that's a step forward.

We are kind of "stuck" on this issue. Just to re-iterate and confirm what's happening:

A Drupal 10.1 site with the 4.0 version of Juicebox installed runs fine ... as far as I can see without plumbing into all corners of Juicebox. I can see existing galleries and I can go into admin/config and run through the image style and Juicebox style settings just fine. Admin/config runs JuicboxFormatter.php. Then using PHPstorm I apply the patch suggested in this issue. As soon as I go back into Admin/config I get

"The website encountered an unexpected error. Please try again later."

Backing out of this and going to admin/reports/recent log messages I get

"TypeError: Drupal\juicebox\JuiceboxFormatter::__construct(): Argument #9 ($file_url_generator) must be of type Drupal\Core\File\FileUrlGeneratorInterface, Drupal\Core\Entity\EntityDisplayRepository given, called in D:\webpage\compg\web\core\lib\Drupal\Component\DependencyInjection\Container.php on line 259 in Drupal\juicebox\JuiceboxFormatter->__construct() (line 109 of D:\webpage\compg\web\modules\contrib\juicebox\src\JuiceboxFormatter.php)."

The patch does seem to alleviate the PHPCS/dependency injection error but it won't do much good until we can figure out this error and fix it.

There are some other patches pending, but this issue is the show stopper for now.

Mahasri Shanmugasundaram → made their first commit to this issue’s fork.

Hello,

Dependency injection issues in JuiceboxFormatter.php file has been fixed in this patch.I can able to access Admin/config page now. Please review.

In #11, what commit? What am I missing?

I am working on this locally. Last night I was focusing on @param statements that feed into the __construct statement. I think that one of the "variables" feeding in through a param into the __construct is not of the right type and thus generates an error. Trying to track this down.

Edit: I guess we were working on this concurrently. So, sorry for my initial response. I see the proposed patch now.

I will try out the suggestions in 3371019-3.patch and report back. It looks promising and may address the issues I am seeing. It is great to have someone else focusing on this. Thanks.

I tried the 3371019-3.patch. No immediate JOY from this. Still getting the error I reported.

Here is the relevant code I'm working with:

At the top the use files:

use Drupal\Core\Messenger\MessengerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Drupal\Core\Path\CurrentPathStack;
use Drupal\Core\Config\ConfigFactoryInterface;
use Drupal\Core\Routing\UrlGeneratorInterface;
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Component\Render\FormattableMarkup;
use Drupal\Component\Utility\Html;
use Drupal\file\FileInterface;
use Drupal\Core\StringTranslation\TranslationInterface;
use Drupal\Core\StringTranslation\StringTranslationTrait;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\Core\Security\TrustedCallbackInterface;
use Drupal\Core\Render\Element;
use Drupal\Core\File\FileUrlGeneratorInterface;

next the class statement:

class JuiceboxFormatter implements JuiceboxFormatterInterface, TrustedCallbackInterface {
  use StringTranslationTrait;

  /**
   * A Drupal configuration factory service.
   *
   * @var \Drupal\Core\Config\ConfigFactoryInterface
   */
  protected ConfigFactoryInterface $configFactory;

  /**
   * A Drupal URL generator service.
   *
   * @var \Drupal\Core\Routing\UrlGeneratorInterface
   */
  protected UrlGeneratorInterface $urlGenerator;

  /**
   * A Drupal module manager service.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   */
  protected ModuleHandlerInterface $moduleManager;
  /**
   * A Drupal current path service.
   *
   * @var \Drupal\Core\Path\CurrentPathStack
   */
  protected CurrentPathStack $currentPathStack;

  /**
   * A Symfony request object for the current request.
   *
   * @var \Symfony\Component\HttpFoundation\Request
   */
  protected Request $request;

  /**
   * Storage of library details as defined by Libraries API.
   *
   * @var array
   */
  static protected array $library = [];

  /**
   * The messenger service.
   *
   * @var \Drupal\Core\Messenger\MessengerInterface
   */
  protected MessengerInterface $messenger;

  /**
   * A Drupal entity type manager service.
   *
   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   */
  protected $entityTypeManager;

  /**
   * The file URL generator.
   *
   * @var \Drupal\Core\File\FileUrlGeneratorInterface
   */
  protected $fileUrlGenerator;

  /**
   * The module handler interface service.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   */
  protected ModuleHandlerInterface $moduleHandler;

Next the params:

/**
   * Constructor.
   *
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The Drupal config factory that can be used to derive global Juicebox
   *   settings.
   * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
   *   A string translation service.
   * @param \Drupal\Core\Routing\UrlGeneratorInterface $url_generator
   *   A URL generator service.
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_manager
   *   A module manager service.
   * @param \Drupal\Core\Path\CurrentPathStack $currentPathStack
   *   A current path service.
   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
   *   The Symfony request stack from which to extract the current request.
   * @param \Drupal\Core\Messenger\MessengerInterface $messenger_interface
   *   The messenger interface.
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager service.
   *  @param \Drupal\Core\File\FileUrlGeneratorInterface $file_url_generator
   *   The file URL generator.
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   *   The module handler interface service.
   */

And finally the constructor:

  public function __construct(ConfigFactoryInterface $config_factory,
  TranslationInterface $string_translation,
  UrlGeneratorInterface $url_generator,
  ModuleHandlerInterface $module_manager,
  CurrentPathStack $currentPathStack,
  RequestStack $request_stack,
  MessengerInterface $messenger_interface,
  EntityTypeManagerInterface $entity_type_manager,
  FileUrlGeneratorInterface $file_url_generator,
    ModuleHandlerInterface $module_handler) {
    $this->configFactory = $config_factory;
    $this->stringTranslation = $string_translation;
    $this->urlGenerator = $url_generator;
    $this->moduleManager = $module_manager;
    $this->currentPathStack = $currentPathStack;
    $this->request = $request_stack->getCurrentRequest();
    $this->messenger = $messenger_interface;
    $this->entityTypeManager = $entity_type_manager;
    $this->fileUrlGenerator = $file_url_generator;
    $this->moduleHandler = $module_handler;
  }

And for some reason I am unable to see, I keep getting:

"TypeError: Drupal\juicebox\JuiceboxFormatter::__construct(): Argument #9 ($file_url_generator) must be of type Drupal\Core\File\FileUrlGeneratorInterface, Drupal\Core\Entity\EntityDisplayRepository given, called in D:\webpage\compg\web\core\lib\Drupal\Component\DependencyInjection\Container.php on line 259 in Drupal\juicebox\JuiceboxFormatter->__construct() (line 123 of D:\webpage\compg\web\modules\contrib\juicebox\src\JuiceboxFormatter.php)."

I also verified that the services.yml file was modified per patch #3. I keep thinking I must have a typo in my code but can't see it. Why $file_url_generator is not of the correct "type" is beyond me at present.

wow. After much gnashing of teeth and many attempts, I wondered, is patch-3 meant to be applied to the files resulting from patch-2 (sorry: 3371019-3.patch and 371019-2.patch respectively) or should I go back to the 4.0 "release" and apply the patches from scratch (ignoring patch-2). It turns out (tentatively) to be the latter approach. I got fresh JuiceboxFormatter.php and services.yml files from the repository and applied the patches and voila: I could run admin/config and all my photo galleries on my test Drupal 10.1.1 site.

So thank you @Mahasri Shanmugasundara. I will be doing further testing including moving a copy of the code to my "production" site and testing it there. But this looks like a breakthrough. And thanks to @arti for getting things rolling.

I hadn't thought of the need to revise the services.yml file prior to your patch. I am still learning about how the various pieces: "services.yml" ... the param statements and the actual constructor fit together. Seems like Drupal Core could do a better job of giving useful error messages when parameters are not in the right order or missing. Telling me that file_url_generator is not of the right type is not very helpful. But that's neither here nor there, I suppose.

We will take care of issuing credits to all who have contributed as we get a new release together. There are some other pending patches that need a bit of work and should be coordinated as part of a release. I'll need (and want) Luke's input on this part.

Caching!!!

Last night (7/21) after getting Juiceboxformatter working with the patch-3 code (including patching the services.yml file), the problem with "Drupal\Core\Entity\EntityDisplayRepository" starting showing up again. I was "gob-smacked" to use a technical term. I checked that I had the latest code (post-patch). Yeah, I did. I was doing a drush cr (cache rebuild) in a terminal window every time I made a change to the code because my previous experience with Drupal shows that caching can cause changes to sometimes not "appear". That didn't help. I tried emptying browser cache. That didn't help either.

I knew that the "old" services file contained an entry for '@entity_display.repository'] where file_url_generator needed to go. But that entry had been changed in services.yml. So why was the error re-occurring?? I know that clearing cache in Drupal can be hit or miss. I'm old and remember when we used to go right into PHPmyadmin and deal with SQL tables so I'm familiar with the way Drupal stores a bunch of cache tables and I've seen that many methods of clearing cache don't really clear those tables. Including I guess Drush cr. So, on my local system I went into PHPmyadmin and emptied all the cache tables (after verifying that there were entries for EntityDisplay in some of them). Immediately the patched version of the files started to work with admin/config.

Time to ride my bicycle. This will take more testing and confirmation but what I think was happening was that entries in the old services.yml were somehow being cached in MYSQL and then causing problems with the __construct parameters.

Yesterday doing more testing on my local system I ran into problems with module_manager versus module_handler. Led to problems such as this:

Deprecated function: Creation of dynamic property Drupal\juicebox\JuiceboxFormatter::$moduleHandler is deprecated in Drupal\juicebox\JuiceboxFormatter->__construct() (line 126

On my test system I've made the problem go away. It appears that the variable Drupal wants us to use is:

 /**
   * A Drupal module manager service.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   */
  protected $moduleManager;

which leads to a param like this:

   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_manager
   *   A module manager service

and then in the construct statement:

ModuleHandlerInterface $module_manager

and finally

$this->moduleManager = $module_manager;

when the variable is finally used down in the guts of the program you get:

$icon_dir = $this->moduleManager->getModule('juicebox')->getPath() . '/images/mimetypes';

which avoids the dependency injection issue. However in the juicebox services.yml file we still have:

arguments: ['@config.factory', '@string_translation', '@url_generator', '@module_handler', '@path.current', '@request_stack', '@messenger', '@entity_type.manager', '@file_url_generator']

for now this all seems to work. Why the heck Drupal sticks underscores in some of the variable names and then has versions without underscores (which are after all completely separate variables afaik) maybe I'll understand someday.

I will keep working on it until we get a single patch we can apply to move the version of Juicebox towards a stable Drupal 10 release.

I think there may be a better way to approach this that will make 4.x much more easy to maintain backwards compatibility for.

Instead of overriding constructors, we should be using the ContainerFactoryPluginInterface pattern to do so.

So we can delete the entire constructor in the class and do this instead:

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {

    $instance = new static(
      $plugin_id,
      $plugin_definition,
      $configuration['field_definition'],
      $configuration['settings'],
      $configuration['label'],
      $configuration['view_mode'],
      $configuration['third_party_settings']
    );

    // Inject any required services here!
    $instance->serviceXXX = $container->get('service_xxx');

    return $instance;
  }

This means that we won't be breaking any b/c contracts whenever service injections happen. It also means fewer lines of code to maintain!

If we don't get traction in a few days, I'll upload a new patch for testing.

Anything that makes this easier to maintain will be welcome. I will be looking forward to studying your patch.

After I got my code "running" locally two days ago I stumbled into other code in the src directory *JuiceBoxGalleryInterface.php and JuiceboxGallery.php for example. Both have a lot of technical debt that appears when viewed in PHPstorm. Some are as simple as spelling errors, others functions without a return type. A few are more complicated to deal with. Look at the function filterMarkup at the bottom of JuiceboxGallery.php for instance. It appears to me that it is using (or "filtering") obsolete html codes.

Locally I've done a bunch of changes to clean up the code. Once we get JuiceBoxFormatter working reliably I'll post some suggestions (and a patch) for this. I have an older pending patch that I need for fix up too. It deals with some other peripheral and cleanup issues. I think we also need to patch the test directory code to be consistent with the code that fixes our dependency injection issues. Since the test code runs Juicebox.module which in turn runs JuiceBoxFormatter we need to do the fixes in a consistent and logical order.

I've added an empty post-update hook to #12 per https://www.drupal.org/node/2960601 → and have marked the helper service @internal (in-case future dependencies need to update after tagging 4.0.0 stable).

Disregard the injection review in #18; I had mistakenly thought the target scope was the Field Formatter Plugin, and not the helper service.

Let's see what the test bot thinks about it.

Aside from a few coding standard "things" the patch looks good.

I applied the patch to my local system. I got a fresh copy of juiceboxformatter.php from the "official" 4.0 repository here to apply the patch to. It took a little fiddling but the patched juiceboxformatter works on my localhost system with Drupal 10.1.1. I want to update my local Drupal to 10.1.2 and test some more.

PHPCS reports a few items. PHPstorm is even more fickle than PHPCS and if we made it totally happy by fixing all the typos and functions that don't have return type declarations etc. there would be many patches. Not sure if the juice is worth the squeeze. PHPstorm does report some items that it might be good to fix such as variables that aren't used.

Bigger picture: we are well on our way to fixing the dependency injection issue and making PHPCS happy on that score.

I think that we should do one huge patch in another issue to resolve all of the standards violations. That's largely very safe and automated these days with phpcbf and the test bot.

Agree with #22 with a couple of qualifications and questions.

Status-wise I updated to 10.1.2 (the latest and greatest Drupal) this morning (August 4, 2023). Then, with JuiceboxFormater.php patched to #20 on Phpstorm locally (and PHPstorm running at the latest release level) I tested on my Wampserver virtual host. Going to admin/configuration has been the acid test of whether this is working and it is! Yeah. I will update my hosted, "production" system with 100+ Juicebox albums in the next couple of days.

So: questions ...

1. Where are we going to apply these patches, including 3371019-20.patch? We have a 4.0.x branch (which I have a copy of locally in my local Git). We have a tagged 4.0.0-alpha1 release, which is what we recommend to people who are willing to test Juicebox with Drupal 10.

2. Patches are dependent on prior patches to the same code, NO? So where do we put the patches from 3371019-20? Any further patches (and there are some) to JuiceboxFormatter.php will have to be based on the code that results from 3371010-20 no?

3. Should we make the branch 4.0.x the default in our project? This would at least reduce the number of questions and changes we get suggested for the old 8.x ... code.

4. We list a dev version on our project page: composer require 'drupal/juicebox:4.0.x-dev@dev' ... but that doesn't appear in our branch or tag listing. I'm sure my confusion here is a result of not fully understanding releases and branches and tags. I'm guessing we want to have a working dev version somewhere to apply the patches as we work toward a stable 10 release. We'd want to maintain the current "caveat emptor" warnings for the dev release.

I think we're good to cut 4.0.0 stable and switching the default branch after merging this, Frank.

fkelly12054@gmail.com → closed merge request !9

I thought I could get
3371019-20.patch
committed by opening a merge request. However that merge request looks like it was going to commit 3371019-3.patch.

These patches are identical except for the update hook in the -20 patch, which deals with a need to refresh cache when the update is done (otherwise the old services.yml code will still be active). I don't know how to essentially rtbc one patch and not the other.

If I'm not mistaken this patch will update the 4.0.0 code for juiceboxformatter and service.yml and provide the basis for us to proceed with other cleanup patches.

I canceled the premature merge.

Committed and pushed to 4.0.x-dev - thanks.

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