Resolve javascript coding standard violations

Mostly automated fixes from phpcs -- I'm assuming we'll need more manual fixes as the test bot illuminates things.

Setting to NR to get the test bot to run.

Let's save the drupal association some costs and halt on fail for these tests to start.

Attempt to resolve some of the few remaining JS violations.

I think we'll need to disable eslint for one line -- nothing we can do about the juicebox api.

7th time's the charm, perhaps?

Okay, so #7 looks like it's passing all linting / coding standards checks that the test bot runs. The javascript took a bit of work to get passing, so we should probably manually test this with some level of scrutiny.

Luke.Leber → credited arti_parmar → .

Moved issue credits from the now-closed https://www.drupal.org/project/juicebox/issues/3371020#comment-15185053 📌 Unused variable $bundle_entity_type. Closed: duplicate

bearing in mind the adage: above all do no harm ...

3351683-coding-standards-7.patch passes the tests. If I understand the process correctly, the next step would be to create an issue fork. Then merge this. Not sure if the merge will automatically create a commit but a commit seems to be needed.

The revised code needs to get into branch 4.0.x right? At that point any developers testing this (including me) can download the revised code to their local repository(s) and test on a local set up with the latest 10.x Drupal version.

Hey Frank, it can be approached two ways. Patch workflow and issue fork workflow can go equally well.

Step one in patch workflow is to pull in 4.0.x-dev via composer (composer require drupal/juicebox:4.0.x-dev). Next, the patch needs to be downloaded locally and applied (or applied via composer patching).

Issue fork workflow can be composer require'd directly, but IIRC you'd have to set up the repository entry in your composer.json file first.

I tend to prefer patch workflow myself, just because it's a lot easier to work on a lot of different issues rapid-fire. I usually have a lot of parallel issues building when I have contrib time slots anymore.

D:\webpage\compg_2>composer require "drupal/juicebox:4.0.x-dev@dev"
./composer.json has been updated
Running composer update drupal/juicebox
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 1 update, 0 removals
  - Upgrading drupal/juicebox (4.0.0-alpha1 => dev-4.0.x 99b564e)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 1 update, 0 removals
  - Syncing drupal/juicebox (dev-4.0.x 99b564e) into cache
  - Removing drupal/juicebox (4.0.0-alpha1)
  - Installing drupal/juicebox (dev-4.0.x 99b564e): Cloning 99b564e1a3 from cache
Package container-interop/container-interop is abandoned, you should avoid using it. Use psr/container instead.
Package zendframework/zend-hydrator is abandoned, you should avoid using it. Use laminas/laminas-hydrator instead.
Package zendframework/zend-servicemanager is abandoned, you should avoid using it. Use laminas/laminas-servicemanager instead.
Package zendframework/zend-stdlib is abandoned, you should avoid using it. Use laminas/laminas-stdlib instead.
Package zendframework/zend-text is abandoned, you should avoid using it. Use laminas/laminas-text instead.
Package behat/mink-goutte-driver is abandoned, you should avoid using it. Use behat/mink-browserkit-driver instead.
Package fabpot/goutte is abandoned, you should avoid using it. Use symfony/browser-kit instead.
Generating autoload files
Hardening vendor directory with .htaccess and web.config files.
80 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
Cleaning installed packages.
Info from https://repo.packagist.org: #StandWithUkraine
No security vulnerability advisories found.

The composer require listed (see above screen capture from Composer terminal session) is not getting the code needed. For instance the info.yml file that results is:

name: Juicebox
type: module
description: 'Provides an integration between the Juicebox HTML5 responsive gallery library and Drupal.'
package: Media
core_version_requirement: ^9.3 || ^10
configure: juicebox.admin_settings
dependencies:
  - drupal:file
  - drupal:image

with no section for the version. This info (the version) is not in the patch file. This info is in the tar.gz file that is associated with the composer require "drupal/juicebox:4.0.x-dev@dev" instruction on the page: https://www.drupal.org/project/juicebox/releases/4.0.x-dev →

A couple of days ago I had manually applied the patches in the coding standards - 7 patch but I wanted to use GIT to apply them "officially".

This process is not going very well. I have quite a few changes that I've experimented with on my local copy of the code in PHPstorm but coordinating these with whatever activity is taking place through these composer and patch processes is difficult. I think we need some kind of "target" branch where patches can be reviewed, rtbc'd and applied ... and then moved on from.

I have been continuing, as time permits to work on the set of programs related to JuiceBoxDisplayStyle.php. This necessitates changes to programs that have been already patched in this issue.

Two issues that go beyond my expertise level are

1. Replacing obsolete references to watchdog_exception (in JuiceboxXmlControllerBase.php, JuiceboxFieldFormatter.php and JuiceBoxDisplayStyle.php

and 2 in JuiceBoxGallery.php there is a function filterMarkup that attempt to make sure that only legitimate HTML codes are in the gallery object. That is seriously out of date but I'm not sure what the best approach to replacing it is. I'm sure we don't want to create a security issue by not filtering the codes in a Gallery, but am not sure how to proceed.

I will have dozens, if not hundreds of patches to apply when all this is working. Many are just typos that Phpstorm is nagging me about or sticking return values on functions or type values on parameters.

Stop overriding constructors (let's see how the test bot does).

Let's try to forget that #15 ever happened.

Ugh...and #16 too. :-)

Resolve violations added by last 2 commits..my bad there.

Postponed on https://www.drupal.org/project/juicebox/issues/3407770 📌 Move continuous integration testing to Gitlab Needs review

Since Gitlab CI is the new long term support CI provider, we should probably fix things against that versus Drupal CI.

I'm un-postponing this issue.

Given we're running on Gitlab now, let's get all of the jobs green. I'll open a fresh MR to start.

luke.leber → changed the visibility of the branch 3351683-pp-1-resolve-all to hidden.

Working on it

I have been using PHPCS locally, with the Juicebox code in a PHPStorm project that is consistent with 4.0.x-dev (or started that way). I made changes that resolved all PHPCS issues using --standard=Drupal. However --standard=DrupalPractice still throws an error no matter what I try. E.g.,

:\webpage\compg_6>phpcs --standard=DrupalPractice web\modules\contrib\juicebox

FILE: ...ontrib\juicebox\src\Plugin\Field\FieldFormatter\JuiceboxFieldFormatter.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
332 | WARNING | \Drupal calls should be avoided in classes, use dependency
| | injection instead
--------------------------------------------------------------------------------

The code I'm looking at is in;

:\webpage\compg_6>phpcs --standard=DrupalPractice web\modules\contrib\juicebox

FILE: ...ontrib\juicebox\src\Plugin\Field\FieldFormatter\JuiceboxFieldFormatter.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
332 | WARNING | \Drupal calls should be avoided in classes, use dependency
| | injection instead
--------------------------------------------------------------------------------

and I've run line by line comparisons between that and the code starting from 4.0.x. Looking through some issue logs, we had similar problems a year ago with code in Juiceboxformatter.php that related to the layout of the services.yml file. Those were resolved and I'm not aware of any recent changes to services.yml.

Building on #29 I decided to look at JuiceboxFormatter.php and see if anything we did in there would help with the Juiceboxfieldformatter.php problems. Voila I saw that we had added some trustedcallback code after the create statement in juiceboxformatter. More specifically:

 public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition): FormatterBase|JuiceboxFieldFormatter|static {
    // Create a new instance of the plugin. This also allows us to extract
    // services from the container and inject them into our plugin via its own
    // constructor as needed.
    return new static($configuration, $plugin_id, $plugin_definition, $container->get('entity_type.manager'), $container->get('link_generator'), $container->get('request_stack'), $container->get('juicebox.formatter'), $container->get('renderer'), $container->get('entity_display.repository'));
  }
  /**
   * {}
   */
  public static function trustedCallbacks(): array {
    return [
      'preRenderFieldsets',
    ];
  }

  /**
   * {@inheritdoc}
   *
   * @throws \Exception
   */

After putting that in, I reran PHPCS standard=DrupalPractice and the "\Drupal calls should be avoided in classes, use dependency
injection instead" error message went away. I need to try this on a website and see if it actually works outside of PHPCS.

This all is beyond my level of expertise and all I'm trying to do it get Juicebox running with Drupal 10 and 11 to give people (including myself) more time to find an alternative.

Just an update. Two days ago I thought I had PHPCS using both standards down to one "Drupal calls should be avoided in classes" error and that I could patch together code from Juiceboxformatter.php into JuiceboxFieldFormatter.php to resolve that. After doing that it seemed to remove the errors from Juiceboxfieldformatter when using --standard=DrupalPractice. But then I reran PHPCS using standard = Drupal and came across a whole bunch of formatting errors that I had reintroduced by making some so-called minor tweaks that PHPstorm pointed out. Sorry if this is getting deep into the weeds for some, but I wanted to explain part of why these updates are taking so long.

The next day, trying to deal with the new "standard=Drupal" errors I somehow reintroduced similar errors in JuiceboxFormatter ... which had been resolved (allegedly) for some time.

Working through the code it appears that we are also dealing with a lot of "technical debt" since this module has not had regular, ongoing attention from a fully qualified Drupal developer (which I am not) for years. Luke has provided patches that have made the product work with Drupal 10 (and I think with Drupal 11 though I've not had time to test that). But there are all sorts of errors in the code such as variables that never get assigned a value and code that attempts to filter out html codes that should not be in a completed xml file that is used by Juicebox. PHPstorm points out obsolete or incomplete html codes in that PHP code but it can't tell us how to fix them.

And, just to emphasize the technical debt further, when I looked up "trustedcallback" to understand more about how it worked, it appears that the developers and core committers are busily in the middle of deprecating it and subtituting in a slightly different approach. See:

https://www.drupal.org/project/drupal/issues/3274867 📌 Add TrustedCallback attribute Fixed and related issues.

I will continue doing what I can to produce a juicebox version that will meet all standards tests for Drupal 10 and 11 and buy us more time. But I increasingly think it is time to move on from Juicebox as a whole and find a slideshow approach that is fully supported and based on media files rather than a bunch of unmanaged files uploaded by FTP.

A question for the user base: does anyone out there use Juicebox features that allegedly build xml files containing sections that have the options for a gallery preceding sections for images and sections listing the thumbnail files? I ask because there is a lot of complicated code that alleges to do that and I'm not sure it's ever used.

Thrashing away at this as time permits. Using the code that was recently uploaded (committed) to "
Issue #3433346: Automated Drupal 11 compatibility fixes for juicebox" I'm fairly sure we still have Drupal calls should be avoided in classes, at the pasted results from PHPCS below show. In other words we don't have all coding standard violations resolved.

C:\webpage\compg_6>phpcs --standard=DrupalPractice web\modules\contrib\juicebox

FILE: ...\web\modules\contrib\juicebox\src\Controller\JuiceboxXmlControllerBase.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 124 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
--------------------------------------------------------------------------------


FILE: ...ontrib\juicebox\src\Plugin\Field\FieldFormatter\JuiceboxFieldFormatter.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 344 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead

Looking for a fix for this, I'm fairly sure that the code in JuiceboxDisplayStyle.php may provide a solution we could adopt. That program has a " public static function create(ContainerInterface ... " which should provide access to services that can be injected. I will see if I can tackle that in the next few days modeling fieldformatter and xmlcontrollerbase display style after those.

For what it's worth, it might save time to run coder/phpcs locally before uploading it the Drupal site? If it doesn't pass phpcs standards locally it's not going to pass in the gitlab test runs.

I've been using a local Wampserver instance with code in PHPstorm to debug Juicebox '4.0.0-alpha2' I realized that I was trying to use my local site for conflicting purposes (I was also looking at alternatives for photo galleries besides Juicebox) and was experimenting with alternative themes. So I bit the bullet and set up a 10.4.1 Drupal site just for Juicebox using what was essentially '4.0.0-alpha2' Plus ... plus referring to PHPstorm based changes to the code. All went "swimmingly" well until I tried to clear cache on the new local site and got:

( ! ) Fatal error: Declaration of Drupal\juicebox\Controller\JuiceboxXmlControllerViewsStyle::access() must be compatible with Drupal\juicebox\Controller\JuiceboxXmlControllerBase::access(): bool in C:\webpage\compg_7\web\modules\contrib\juicebox\src\Controller\JuiceboxXmlControllerViewsStyle.php on line 63

Using suggestions from Phpstorm I was able to at least temporarily suppress this error. However this led me to a 7 or 8 year old thread on the issue queues

https://www.drupal.org/node/2849674 →

which basically told me that the beginning section use statements in the viewstyle program may be problematic. And, from working recently with the src/controller code directory in Juicebox I am not sure that this whole section of Juicebox code does anything useful.

In the original Juicebox code, some 10 years ago, the basic thought (I believe) was that you'd build your galleries outside of Drupal using JuiceboxBuilderPro software on a PC or Mac. Then upload the xml files, which contained "specs" or "options" for a Gallery and then a list of graphics (jpg's usually) files to be processed by the Juicebox.js file. After that, I suspect, it was thought that "maybe it would be a good idea to build the xml file totally within Drupal". I'm not sure if anyone ever used those capabilities but there are some complex programs that allegedly make it possible.

So basically, I can "make" the error I showed above go away but I'm not sure after reading through 2849674 that it's worth the effort. And then, if you look at the comment in JuiceboxXmlControllerBase. php (line 91 and following):

public function xmlController(): Response {
    $xml = '';
    // If we have xml-source query parameters this indicates that the XML can
    // probably not be generated here from scratch. Instead we must depend on a
    // sub-request to another Drupal path (e.g., the gallery page) and search
    // for embedded XML there. This is an experimental method for special cases

It makes me wonder if we aren't chasing our own tail here a bit, trying to fix code that's never even used. This may not be definitive but PHPstorm seems to thing that the function xmlController() is never even used.

Today, 1/27/2025, I finally got a "clean" run of coder/PHPCS on my local Wampserver system ... both with standard=Drupal and standard=DrupalPractice. That is one step towards resolving all coding standard violations.

Next, I am going to do some functional testing on my local system, which means first copying some Juicebox galleries from my so-called "production" system at fkelly.org and trying out all the Juicebox functionality that I've used.

Quite honestly there is a code "stench" (for want of a better word in the whole src/controller directory). Tnere are even comments in the 7+ year old code that indicate the original author may have thought the code was not / or even may never be used. Just for instance see the xmlcontroller function in JuiceboxXmlControllerBase.php which says outright:

// If we have xml-source query parameters this indicates that the XML can
    // probably not be generated here from scratch. Instead, we must depend on a
    // sub-request to another Drupal path (e.g., the gallery page) and search
    // for embedded XML there. This is an experimental method for special cases.

I suspect that if we had a way to determine whether the code was reachable, we might be able to eliminate a bunch of "junk". But, with thousands of possible Juicebox users and no way to tell who uses what or how often, that's a perilous step to take.
I will report back.