Comments

xeM8VfDh created an issue. See original summary.

neslee canil pinto’s picture

Status: Active » Closed (won't fix)
xem8vfdh’s picture

@Neslee Canil Pinto, so no explanation then? Much of the Libraries module functionality is included in a core library api now, and the Libraries module appears to be largely unsupported. I dropped Juicebox for Colorbox for this reason, among others. I presume other site admins who want to stay current and move to drupal 9 will do the same.

Its really great that you are picking up work on this project, because this is the best gallery module IMO. But, for a module to be useful it needs consistent maintainership and a commitment to forward compatibility, not just slick functionality--just something to consider. Either way, keep up the good work.

neslee canil pinto’s picture

@xeM8VfDh, for now i am thinking of moving forward with what we have. Not changing the existing codes and stuffs. And ya Juicbox is a great module.
I have been maintaining Colorbox too, i have much more interest in projects like this. So taking up the responsbiltiy for it. Can you help me in #3128333: Tests are broken in 8.x-2.x-dev. And ya i have to make Juicebox Drupal 9 ready too. So little bit busy with other prior issues.

xem8vfdh’s picture

@Neslee Canil Pinto, I will take a look at #3128333 when I get a chance.

neslee canil pinto’s picture

Status: Closed (won't fix) » Needs work
xem8vfdh’s picture

hahaha re-opening eh? You all have been quite active. Dropping this would be a nice addition to forward compatibility.

neslee canil pinto’s picture

@xeM8VfDh yes. Libraries module is not maintained now. So we need to do it. Its blocking drupal 9 release. Patches and reviews most welcomed

xem8vfdh’s picture

I agree 100%

handkerchief’s picture

+1

fkelly12054@gmail.com’s picture

I have been following on the periphery of this issue as well as issue:

https://www.drupal.org/project/juicebox/issues/3042821

On a local system (Wampserver and using virtual host) I have a Drupal 8.9.3 system with Juicebox. I installed the latest Juicebox (n: 8.x-2.x-dev) and even managed to apply the patches in 3042821-23.patch. Still getting library related errors, leading me to research in the libraries api thread:

https://www.drupal.org/project/libraries

where I even managed to find and install

https://www.drupal.org/project/libraries/releases/8.x-3.x-dev

Which does not solve any problems. Because as @xem8vfdh noted earlier in this thread the contrib libraries is unsupported and a dead end. It is never going to be supported in Drupal 9, which is where we need to get to. This is evidenced in:

https://www.drupal.org/project/anchor_link/issues/3114086

and more importantly in:

https://www.drupal.org/project/libraries/issues/3110305

where core committer @Alexpotts explains the steps forward, which basically mean getting rid of the contrib module libraries.

But, wait, Juicebox currently requires libraries, so we need a release of Juicebox that doesn't have this requirement.

As I understand Alex's post, we SHOULD be able to run Juicebox in a Drupal 8.9 series release while remaining bugs related to Drupal 9 support are worked out. But I'm not sure what's needed to make Juicebox run with the library support that's built into core as of Drupal 8.9.x.

I'm continuing to see if I can make something work on my Wampserver/Localhost system but obviously the path forward requires maintainer support ... maybe a version that works with 8.9.x and then a fully Drupal 9 version.

fkelly12054@gmail.com’s picture

Based on #11 I seem to have this working. Please excuse a total newbie at Drupal coding for hacking at this but:

1. On my Drupal 8.9.3 site, Wampserver, Localhost I Drush disabled the contrib libraries module AND the Juicebox module.

2. I modified juicebox info.yml to remove the dependency on libraries.

3. On juicebox.install I found another module named panelbutton that had already gone through the conversion mentioned in #11. I plagarized their code ... I'm not skilled enough to write it from scratch. In juicebox install I did this:

 if ($phase == 'runtime') {
    // Get locally installed library details.
    if (\Drupal::hasService('library.libraries_directory_file_finder')) {
    /** @var \Drupal\Core\Asset\LibrariesDirectoryFileFinder $library_file_finder */
    $path = \Drupal::service('library.libraries_directory_file_finder')->find('juicebox/juicebox.js');
  }
  else {
    $path = DRUPAL_ROOT . '/libraries/juicebox/juicebox.js';
  }
  if (!file_exists($path)) {
    $requirements['juicebox'] = [
      'title' => t('Juicebox library is missing'),
      'value' => t('Plugin not detected'),
      'severity' => REQUIREMENT_ERROR,
      'description' => t('Juicebox requires the juicebox.js library.
              (/libraries/juicebox)'),
    ];
  }
  else {
    $requirements['juicebox'] = [
      'title' => t('Juicebox'),
      'value' => t('Plugin detected'),
      'severity' => REQUIREMENT_OK,
    ];
    }
  }
  return $requirements;

I then re-enabled Juicebox and, to my surprise, it worked. The panelbutton code has an escape hatch to allow for situations where the contrib library module is still installed (pre-Drupal 8.9.x I think). I'm not interested in that so I removed it.

If this approach proves to work, I would suggest:

first, get a version of Juicebox that works at 8.9.x and above with the libraries module disabled. Then figure out whatever Drupal 9 blockers remain and fix those in a separate issue.

xem8vfdh’s picture

nice, thanks for the contribution @fkelly, seems like a reasonable enough approach!

fkelly12054@gmail.com’s picture

While I can "run" juicebox and view existing galleries with this approach there are still a number of other areas where the code will need to be adapted to use the revised library detection methods. For instance, in juiceboxformatter.php there is a getLibrary function that uses the old methods of finding the library ... I think it then tries to pass some information over to settingsform.php that is not in the format settingsform expects, causing errors ... of course. No actually juiceboxformatter will fail completely with the old approach, I tried to modify it to use the new approach but have my variable types mixed up ... I think.

Someone with more expertise needs to go through this all can revise the library detection to use the new (core based) approach.

xem8vfdh’s picture

I believe everyone above is working on it :)

neslee canil pinto’s picture

@fkelly @xeM8VfDh yes. Will upload a patch in a couple of days.

neslee canil pinto’s picture

StatusFileSize
new1.89 KB

Just a simple starting patch. Long way to go.

neslee canil pinto’s picture

StatusFileSize
new8.13 KB
fkelly12054@gmail.com’s picture

I would like to help move this project along. While I have a background with PHP, I don't know the Drupal API very well. I have set up a GIT repository locally based on the code in 8.x-2.x-dev but am finding that applying the patches is not very straightforward. If I am not mistaken the patches need to be applied in a strict order and some of the needed patches are not in this issue but rather listed elsewhere ... e.g.,

$ git-apply 3042821-23.patch

works okay but I can't get the patches in this issue to apply. I suspect I'm missing prerequisites.

I don't think I can be of much help with the coding: Drupal has very strict standards but if I can keep up with the patches then I am a whiz at setting up a local test environment and I have loads of Juicebox galleries to test with. Also wondering if it would be preferable to have one issue: something like "make Juicebox work on Drupal 9" which would include Removing library dependency as a subset, so that all the patches could be in one place. With maybe periodic commits of dev versions so that everyone could level set as we go and be sure of working on the same code base.

Or if there is some other way to list all the patches since the July 29 commit and the order they need to be applied?

As always thanks for your efforts moving this.

fkelly12054@gmail.com’s picture

Just to extend #19 a little:

I've read up on patch naming and understand that better. So patch 3042821-23 comes from the Drupal 9 Deprecated Code Report issue. Meanwhile 3150320-18.patch comes from the Drop Libraries Dependency issue. But they are doing slightly different things, say to the juicebox.info.yml file. And since the 3042821 patch came first chronologically, I can't even see how 3150320-18 can be applied if 3042821 was applied first. And 3042821 is clearly needed because the contrib module libraries dependency needs fixing before we can move on.

fkelly12054@gmail.com’s picture

I think I have enough of the patched code running to look at some issues. The program juiceboxformatter.php has been modified to attempt to use the new libraries search facility. The function in question looks like this:

public function getLibrary($force_local = FALSE, $reset = FALSE) {
    // We use our own static cache to lazy-load the lib. Libraries API detection
    // has a static cache, but as we may be bypassing full local detection in
    // certain situations, we can't always use it.
    $library = &self::$library;
    if (!$library || $reset) {
      // See if we have been passed version details in the URL. If so we bypass
      // local detection and build our own libraries array.
      $query = \Drupal::request()->query->all();
      if (!empty($query['jb-version']) && !$force_local) {
        juicebox_library_info($library);
        $version_number = Html::escape($query['jb-version']);
        if (!empty($query['jb-pro'])) {
          $library['pro'] = TRUE;
          $version = 'Pro';
        }
        else {
          $version = 'Lite';
        }
        $library['version'] = $version . ' ' . $version_number;
        juicebox_library_post_detect($library);
      }
      // Otherwise we just use the Libraries API to detect the local lib.
      else {
        $library = \Drupal::service('libraries.manager')->load('juicebox');
      }
    }
    $library = 'libraries/juicebox/juicebox.js';
    return $library;
  }

I added the forced override of the variable $library "$library = 'libraries/juicebox/juicebox.js';" after moving a copy of the juicebox library from sites/all to the root /libraries directory. The statement " $library = \Drupal::service('libraries.manager')->load('juicebox');" generates an error so it must not be the correct way to access the libraries manager that comes with core starting in 8.9.x. The error is:

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

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "libraries.manager". in Drupal\Component\DependencyInjection\Container->get() (line 153 of core\lib\Drupal\Component\DependencyInjection\Container.php).

Drupal::service('libraries.manager') (Line: 172)
Drupal\juicebox\JuiceboxFormatter->getLibrary(1, 1) (Line: 19)
juicebox_requirements('runtime')
call_user_func_array('juicebox_requirements', Array) (Line: 403)
Drupal\Core\Extension\ModuleHandler->invokeAll('requirements', Array) (Line: 111)
Drupal\system\SystemManager->listRequirements() (Line: 95)
Drupal\system\SystemManager->checkRequirements() (Line: 121)
Drupal\system\Controller\SystemController->overview('system.admin_config')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)

It looks to me like other contrib modules (see panelbutton) have recently modified their code to access the new approach to libraries.  I'm not quite sure how to adopt their approach.  If I comment out 
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
  

If I "force feed" a fixed value into $libraries and comment out the access to library.manager the config screens at least run ... though there are still errors further on. Seems like this is not a working approach ... contrib modules such as panelbutton may have code that works that we can clone?

I will keep looking but I am no where near the skill level needed. Juicebox would be a great module to have for Drupal 9.

fkelly12054@gmail.com’s picture

This is a real quagmire that hopefully someone more expert can take the lead on.

Here is what the variable $libraries should look like after it is properly detected:

this is what the libaries variable looks like when properly detected
D:\webpage\public_html\drupal8\modules\juicebox\src\JuiceboxFormatter.php:165:
array (size=21)
'name' => string 'Juicebox' (length=8)
'vendor url' => string 'http://www.juicebox.net/' (length=24)
'download url' => string 'http://www.juicebox.net/download/' (length=33)
'version arguments' =>
array (size=3)
'file' => string 'juicebox.js' (length=11)
'pattern' => string '/Juicebox.([a-zA-Z]+[0-9\.\ -]+)/' (length=33)
'lines' => int 5
'files' =>
array (size=1)
'js' =>
array (size=1)
'juicebox.js' =>
array (size=2)
...
'callbacks' =>
array (size=5)
'info' =>
array (size=2)
0 => string 'libraries_prepare_files' (length=23)
1 => string 'juicebox_library_add_info' (length=25)
'post-detect' =>
array (size=2)
0 => string 'libraries_detect_dependencies' (length=29)
1 => string 'juicebox_library_post_detect' (length=28)
'pre-detect' =>
array (size=0)
empty
'pre-load' =>
array (size=0)
empty
'post-load' =>
array (size=0)
empty
'module' => string 'juicebox' (length=8)
'machine name' => string 'juicebox' (length=8)
'path' => string '' (length=0)
'library path' => string 'sites/all/libraries/juicebox' (length=28)
'version callback' => string 'libraries_get_version' (length=21)
'dependencies' =>
array (size=0)
empty
'variants' =>
array (size=0)
empty
'versions' =>
array (size=0)
empty
'integration files' =>
array (size=0)
empty
'disallowed_conf' =>
array (size=0)
empty
'compatible_mimetypes' =>
array (size=3)
0 => string 'image/gif' (length=9)
1 => string 'image/jpeg' (length=10)
2 => string 'image/png' (length=9)
'base_languagelist' => string 'Show Thumbnails|Hide Thumbnails|Expand Gallery|Close Gallery|Open Image in New Window|Images|Next Image|Previous Image|Play Audio|Pause Audio|Show Information|Hide Information|Start AutoPlay|Stop AutoPlay|AutoPlay ON|AutoPlay OFF|Go Back|Buy this Image|Share on Facebook|Share on Twitter|Share on Google+|Share on Pinterest|Share on Tumblr|of' (length=342)
'installed' => boolean true
'version' => string 'Pro 1.4.4.1' (length=11)
'pro' => boolean true

Unfortunately, in the current status $libraries detection is spread over the contrib module libraries, over the new core approach where you would do something like:

if (\Drupal::hasService('library.libraries_directory_file_finder')) {
    /** @var \Drupal\Core\Asset\LibrariesDirectoryFileFinder $library_file_finder */
    $path = \Drupal::service('library.libraries_directory_file_finder')->find('panelbutton/plugin.js');
  }

Substituting Juicebox for panelbutton in the above, except this just finds the path and doesn't fill the $libraries variable.

Also, the juicebox.module file itself detects the library and even "hard-codes" in some of the content in it (see the juicebox_library_info_alter, juicebox_libraries_info, juicebox_library_add_info, and juicebox_library_post_detect functions in juicebox.module.

I'm not sure where the contents of $library are supposed to be generated. Whether we put the downloaded (from Juicebox.net) files in the root/libraries directory (seems to be the recommended approach going forward) or /sites/all/juicebox (the old and current approach) I can't see that these files really tell us what the $library values are supposed to be. Could we just put the values in a libraries.info.yml file, maybe with sections for each "version"? and skip having three ways to auto-detect the library? Unless the values are somehow miraculously embedded in the juicebox.net juicebox.js file (which is minified and thus unreadable to a human) I cannot tell where the heck they are coming from.

I am going to continue working to see if I can make the new "core" facilities work. But we will continue to need to support the detection in the contrib libraries module as a fall back until everyone is at least at 8.9.x. The new core approach will never be backported to Drupal 7.

fkelly12054@gmail.com’s picture

Just a quick note ... the juicebox.module program gets the library info that I've quoted in previous posts from the juicebox.js file that is in the libraries directory. But the only thing it really gets is the version information which is in plain text at the top of the file. Then, depending on the version info and whether it's a PRO or Lite version it fills in the rest of the library configuration information right in the program. I confirmed this by deleting all the rest of the data beyond line 13 and saving the file. And double confirmed it by changing the version at the top of the js file to 1.5.2 (which of course doesn't exist) and the var_dump now shows the version as 1.5.2. Of course I'll restore the js file to its original content.

Once we get this working we should document a bit more how it works. It looks a lot more complicated than it really is ... most of the configuration stuff is basically hardcoded in the Juicebox module programs based off of version.

fkelly12054@gmail.com’s picture

StatusFileSize
new14.78 KB

I am continuing to work through this drop libraries dependency issue. It's a slog. My objective is to allow the contrib modules approach to constructing a juicebox library while migrating to the core replacement for libraries when a site has reached Drupal 8.9 or above. As stated in some of the links earlier in this thread, it does not appear that the contrib libraries approach will work once a user moves to Drupal 9. My approach (or an improved equivalent by a more skilled programmer) should work for that.

The contrib libraries module did a lot of heavy lifting for a number of years and we should be thankful for that. At the same time, and working through the code, it's pretty clear that the juicebox.module in the root juicebox directory actually applied most of the "settings" that were needed based on what Juicebox needs. Two variables: $library and $libraries need to be carefully constructed.

Aside from the juicebox.module program, the juiceboxformatter program acts as the "driver" for constructing these variables. I've included both these programs plus a juicebox.info.yml file (to demo my starting point) and a SettingsForm.php in the attached zip file.

With these revised files I can step through Juicebox on my VirtualHost (Wampserver) system, go into settings and view galleries. I have not yet tested it with other versions of the Juicebox library (I use JS file version 1.5.1 Pro).

Longer range I'm not sure how much of a contribution the 'library.libraries_directory_file_finder' that's now part of core really makes. Libraries are going to go into /libraries or the /sites/all/libraries directory anyway ... I'm not sure why we need to rely on a core service to figure that out for us. But I used it for now.

See how you do with the zip file attached on your disposable or backed up test system and let me know.

fkelly12054@gmail.com’s picture

I know you are all waiting with baited breath. So, after yesterday's post I got to questioning. If we are going to develop an alternative means of loading up the $libraries variable using JuiceboxFormatter as the driver and Juicebox.module as the "vehicle" and if that produces the same results as going through the contrib libraries module, why support the contrib module or make it an option anymore? It's just more code to maintain and more complication. Then looking at the juicebox.module I started asking "why have all these separate functions to construct a $library array". I could see why in the context of a separate contrib module but not with all the functionality embedded in Juicebox module.

And, as I hinted yesterday ... library.libraries_directory_file_finder doesn't really do anything for us ,,, in fact it doesn't even work properly if we have our library in sites/all/libraries. So, the new section of Juiceboxformatter.php looks like this:

    if (file_exists(DRUPAL_ROOT  . '/' . 'sites/all/libraries/juicebox/juicebox.js')) {
             $librarypath= 'sites/all/libraries/juicebox/juicebox.js'; }
        else
          if (file_exists(DRUPAL_ROOT . '/' . '/libraries/juicebox/juicebox.js')) {
          $librarypath= '/libraries/juicebox/juicebox.js';
           }
        if(isset($librarypath)) {
        //  var_dump($librarypath);
          juicebox_build_library_array($librarypath, $library);
        }
        else { die('ryfm');
    }
      return $library;
  }

I'll replace ryfm with something more appropriate once I figure out how Drupal deals with dead ends. If users don't have their libraries in the right place for Juicebox, we want to send them back to fix it. The juicebox_build_library_array function is in juicebox_module and replaces about a half dozen functions that each did a piece of building $library.

After a few hours work today I have it to the point where it builds the $library array and can pass it over to the configuration and then the juicebox settings screen without errors. Next I will try it with other version and pro/lite combinations.

Worth noting that this code "gives precedence" to libraries that are in sites/all/libraries/juicebox and only secondarily will work with libraries in /libraries. That makes the most sense to me since "legacy" juicebox users will probably have their libraries in sites/all.

Feel free to post any suggestions/questions.

fkelly12054@gmail.com’s picture

neslee canil pinto’s picture

@fkelly great work, i will work on creating a patch for it. And test in my local too.

fkelly12054@gmail.com’s picture

StatusFileSize
new10.47 KB

@neslee ... I deleted the previous zip file. I am adding a new one that includes juiceboxformatter.php and the juicebox.module. These are the only two files you should need to use to create a patch. I included the info.yml file in the zip to show what my starting point was.

fkelly12054@gmail.com’s picture

This morning I did some added testing on the files I just sent up. I can use different versions of the library (I tested 1.4.4 and 1.51) and both PRO and Lite configurations. I tested settings both from the configuration/juicebox approach and also from the configuration screen you get when you to into extensions and look under the Juicebox module. On my production system all my juicebox albums are embedded in a Juicebox_gallery content type. I also tried a different content type where I used an image field and then used the Juicebox Formatter on that field. The appropriate configuration options came up for that.

It should be relatively simple to test on a Wamp type server with virtual host without undermining a production system. If I'm not missing something: just replace the two files and have the juicebox library in /libraries or sites/all/libraries.

Thanks for your efforts. I'll keep working on my skill set so I can help more in the future.

fkelly12054@gmail.com’s picture

StatusFileSize
new2.61 KB
new5.04 KB

Maybe try these patches for Juiceboxformatter (3150320-19) and the juicebox.module (3150320-20). I'm teaching myself GIT and patching as I go.

I may be mistaken but this might be all that's needed to remove the libraries dependency. I believe the other patches in this thread have already been applied to my local git repository.

fkelly12054@gmail.com’s picture

Obviously, the patch for Juiceboxformatter failed to apply and the one for the juicebox module applied but failed most of the tests. I will keep working on these, but help is welcome.

The one ISSUE that I was looking at today was what to do if the library is not found. I had put a simple "die" statement into juiceboxformatter but I know that's not the way Drupal would do things. Today I tried a couple of options around using the $notification_top variable from the confBaseForm function that's at the bottom of juiceboxformatter. One version of the code looked like this:

      $notification_top = t('The Juicebox Javascript library does not appear to be installed. Please download and install the most recent version of the Juicebox library.');
                 $this->messenger->addError($notification_top);
                  die($notification_top);

I added the die later, at first I was just passing it to configuration form. And that passing worked but after it showed a nice message at the top of the form it then showed a bunch of errors because the form variables were not present.

Oh, I'm also working on bringing the code up to snuff with Drupal coding standards.

And when I tried to use other areas of Juicebox such as displaying an existing gallery I just got a blank screen where the gallery should be with no messages. I think that want we want is some UNIVERSAL approach where if you try to do anything in Juicebox without having a valid Gallery installed you just get the "library does not appear to be installed message" and are directed back to your home page or somewhere else on your site until you remedy the problem.

fkelly12054@gmail.com’s picture

StatusFileSize
new2.53 KB

Let's see if 3150320-21 patch applies. Looking through the various patches it appears to me that the change for the messenger service near the top of the file (line 104) may have already been made by a different patch and caused this version of the patch to not apply.

The "die" message is still not what we want ultimately but having this patch in will at least allow us to test the new logic that removes the contrib library module from being involved with the generation of the $library variable and allows the new function for constructing the variable that is embedded in juicebox module (that patch applied) to become operational.

Fixing the tests is a bit beyond my salary grade right now but I can learn if need be.

neslee canil pinto’s picture

@fkelly you can combine both Juiceboxformatter and juicebox.module in a single patch. Thank you.

fkelly12054@gmail.com’s picture

StatusFileSize
new7.56 KB

@neslee ... here is 3150320-22.patch which attempts to combine the two patches.

the juicebox module part of the patch looks like it applies. I think I found the error in the jucieboxformatter patch. If this doesn't work I may go back to GIT and recreate the patches from scratch.

Sorry about the learning curve part of this.

As I've mentioned in several posts I think we want a common approach throughout the module about what to do if the library is not set up correctly. Users should not be able to proceed to do anything Juicebox without the library being set correctly. It's a pretty easy determination to make: the juicebox.js file is in one of two locations and if not ... send them to the documentation and bring them to a halt. Probably want the code for that in a single place. Of course, that's if you conclude that we're going in the right direction here with eliminating the contrib library.

neslee canil pinto’s picture

@fkelly yes better to create a patch from scratch without applying even my patch, that will give you a clean build pass.

fkelly12054@gmail.com’s picture

StatusFileSize
new10.91 KB

Let's see if 3150320-24.patch applies. I started from scratch with a new local git repo based on 8.x-2.x-dev from this project.

neslee canil pinto’s picture

@fkelly I think we also have to modify tests.

fkelly12054@gmail.com’s picture

@neslee ... Yes I see that about the tests. It looks like we use Messenger in the Module and Juiceboxformatter but maybe need to have it in the tests also?

I am not familiar with the testing process.

I will start with the coding standards list they gave us and fix those. That appears to be within my capabilities.

I'll be happy to work on anything else you want to assign. We haven't really had an in-depth (or any) discussion about the viability of my proposed approach to eliminating the library dependency. We should be able to test that at this point I think.

fkelly12054@gmail.com’s picture

StatusFileSize
new4.96 KB

Coding standards patch 3150320-25.patch.

fkelly12054@gmail.com’s picture

https://www.drupal.org/node/2931730

Looking at tests, it appears that the search_api module recently modified their tests to deal with the new messenger service. See link above and the patches resulting. Not at all clear to me how we'd implement that in our tests.

neslee canil pinto’s picture

@fkelly patch failed to apply because of whitespace. You can use phpcs to run the drupal coding standards issue.

fkelly12054@gmail.com’s picture

StatusFileSize
new4.95 KB

I think I found the extra whitespace and removed it. I'll upload again, then look into phpcs.

neslee canil pinto’s picture

@fkelly again, the whitespaces are still there.

fkelly12054@gmail.com’s picture

StatusFileSize
new4.96 KB

lets try a version with an ending line feed. Ran various versions locally until I came up with 3150320-27.patch

for what it's worth I've been creating patches from commits and extracting what I think I need in an editor. Unfortunately, manual edits are not reliable. I have to figure out how get git to create my patch files directly. I've been trying to use gitk in conjunction with git-gui. The option of creating a patch directly is often greyed out. I guess I may need to do it directly from the command line.

I did see the whitespace you mentioned and eliminated it.

Edit ...
10/26 working from a "virgin" 8.x-2.x-dev download saved into a Git locally and then updating with juiceboxformatter.php and juicebox.module files I am still running into whitespace problems ... and a couple of other issues. Have learned to "apply" them locally and look at any errors generated there. No sense uploading them to Drupal until they apply cleanly on my local git. I'm generating the patch files from the commits of the two files (formatter and module). I think I should be able to eliminate the whitespace problems. There may still be a couple of persistent errors in patch that will need investigation.

Stay tuned.

fkelly12054@gmail.com’s picture

StatusFileSize
new10.13 KB

Nuts! I have gone through probably a half dozen attempts to get the patch for juicebox.module to apply locally ... including deleting and reloading the local git repository that is clones from the 8.x-2-dev repository here. Once I have the repository initially loaded the juicebox.module file matches exactly the one in the repository here. I can then edit that file and replace its content with the new version I've developed. I can then commit the changes to juicebox.module to my repository. The changes look good. I can run a file comparison tool I have named Beyond Compare and the module in my development area matches exactly the one that's in my git repository. But then I create a patch and the patch does not apply. I get an error message like this:

D:\webpage\jboxgit\juicebox>git apply 3150320-32.patch --verbose
Checking patch juicebox.module...
error: while searching for:
   }
 }

This is at line 52 in the patch. I've tried hand editing the patch ... the code has four spaces before the '}" but the patch always throws in 3. Nothing works and I've spent days on it.

I know that the standard procedure is to upload a patch file that works. If I could do this I would. It also occurs to me that what we are really trying to do is simply replace the juicebox.module and juiceboxformatter.php files that are in the 8.x-2-dev distribution. I put the new files in a zip.

I will keep working to see if there is another approach. I don't know why the commit of the files works and the patch based on the commit doesn't!!! I do know that the new code works on my system that has 50+ Juicebox albums. I can't swear that there will not be problems found somewhere deeper inside of the Juicebox code since I don't exercise every option on my systems. And obviously, even when we get this working the test code is going to need updating. I'm not sure that's been touched in years.

Either @neslee or someone else with more Git expertise is going to have to help if we're going to move this forward. Or just replace the files and create a new version tar.gz.

I'd love to see Juicebox running on Drupal 9 and up so I'm not giving up. Substantively, @XE... was perfectly right when he started this issue up. Using the contrib libraries module is way overkill and much more complicated than what we really need to construct a simple $library variable. Not to mention that that module won't ever work with Drupal 9 and up.

neslee canil pinto’s picture

StatusFileSize
new13.42 KB
neslee canil pinto’s picture

In backend juicebox settings is working fine. Applied a patch which you can apply it to your local by using command

curl patch-file-url | git apply - 
neslee canil pinto’s picture

StatusFileSize
new14.69 KB

This patch works and removes the complete dependency from libraries. This is great, I guess we are close to the release of the Drupal 9 version for this module. Just needs some people to test it.

And I think we need to rewrite the tests for this module, so I am going to delete the entire tests folder and recreate from the starting, Help will be appreciated.

neslee canil pinto’s picture

What are your thoughts about creating a new branch(8.x-3.x) for drupal 8/9 with no libraries has dependency? So 8.x-2.x will be still supported with libraries as a dependency for only drupal 8

fkelly12054@gmail.com’s picture

It's magic! Thank you @Neslee. Note for other newbies: it appears you can't apply the patch in #46 and then the one in #48. The one in #48 is more comprehensive so, subject to guidance from Neslee, use that only on a new git library based on 8.x-2.x.

Yes, having a new branch, 8.x-3.x, would be a great starting point for people. Maybe create a new issue to go along with that so people don't have to wade through this entire issue moving forward. We'll want to update the documentation on the project page to go along with this ... and I never completed the messages we generate if someone does not have the correct files in either /library/juicebox or sites/all/libraries/juicebox. I'm not sure what the proper way to do that is in a Drupal context ... obviously we don't want a simple "die" statement. Probably ANY EFFORT TO use any of the juicebox module programs should centrally detect this and give a "user friendly" but firm message directing them to setup instructions?

fkelly12054@gmail.com’s picture

Looking at my system after applying 3150320-48.patch and after various other changes ... I'm not sure if this 9 (patch 48) gets rid of juicebox_library_add_info and function juicebox_library_post_detect. Neither of those functions should be required in juicebox.module after these changes. I'm getting ready to put the changed files into my "live" site (with backups first of course) to see if any problems surface. They shouldn't because my live site is a duplicate of what I test with.

If it would be possible to have more frequent commits that would be great. It gets mighty confusing when there are a bunch of proposed patches lined up.

D4K0’s picture

fkelly12054@gmail.com’s picture

Please see:

https://www.drupal.org/project/juicebox/issues/3184078

But just to make sure to update this thread ... a couple of us are testing the post 8.x-2.x.dev code on a separate github server.

We've just found that the libraries js files are not being loaded properly (or at all) in the case where a gallery is created on-site using the Juicebox formatter. When the contrib libraries module was being used it had a couple of library loading functions, so that after the library was detected it would be loaded. It may have been using deprecated functions to do this ... not the recommended approach for getting from Drupal 8 to Drupal 9. Anyway, the discussion continues in the thread (issue) above. Do not rely on a github clone from the Drupal.org juicebox module. You will get obsolete stuff (as of 12/8/2020). We hope to rectify that soon.

fkelly12054@gmail.com’s picture

https://www.drupal.org/project/drupal/issues/667058
add a libraries folder with a readme.txt to Drupal Root
https://www.drupal.org/node/422996
policy on 3rd party assets
https://www.drupal.org/node/2947530
including 3rd party assets
https://www.drupal.org/docs/7/modules/libraries-api/installing-an-extern...

Above are references to many discussions about libraries, going back almost a decade. As best I can determine, these discussions have been inconclusive. And they make reference frequently to what I believe is the now deprecated contrib libraries module.

I (fkelly) don't intend to go down this "rabbit hole". I am JUST looking for a solution to get one javascript minified file included the correct way into the Juicebox module going forward to Drupal 9. We don't want to violate the Drupal Policy on 3rd party assets. We don't want to do something that is technically incorrect or that will be obsoleted in the near future.

We have a working solution over at:
https://www.drupal.org/project/juicebox/issues/3184078

on my github site that's referenced in issue 3184078. I'm not really happy with it though. I wonder if a more reliable solution might be to (1) standardize a location for the juicebox library files for anyone moving to the new release and (2) just include it on every page load through the libraries.yml file. We would have to see if this causes any problems with the instructions for embedding Juicebox galleries that are built externally and manually uploaded (FTP). These have a statement that loads the library. That might cause problems. Or not? We don't want to force users to have to go back through all their gallery content items and modify (view source) their embeds (in my case that would involve manually editing over 50 content items.

I'm not even sure how to go about getting direction on this. Have our maintainer talk to a core committer or two or someone else with authority in Drupal.org? Just adding another post to an inconclusive issue queue (see above list) does not seem like a productive way to go.

neslee canil pinto’s picture

Version: 8.x-2.x-dev » 8.x-3.x-dev
Status: Needs work » Needs review

New branch and release has been made for drupal 9 support, please test - https://www.drupal.org/project/juicebox/releases/8.x-3.0-alpha1

neslee canil pinto’s picture

Thanks for the work flekky and DK

neslee canil pinto’s picture

Status: Needs review » Fixed
lpsolit’s picture

Great job, really! I installed 3.0-alpha1 on Drupal 8.9.11 and it works great! I deleted the Libraries module to make sure it wasn't used.

The only problem I see is in the log. Everytime I view a gallery, I see this message:

Page not found /drupal/modules/juicebox/component?qlg4wb

I don't know what "component" is supposed to be, but this file doesn't exist.

fkelly12054@gmail.com’s picture

I don't know what "component" is supposed to be, but this file doesn't exist.

Yes, I saw a variant of that message. The "component" is a css file. Under the top level juicebox directory for the module there is a css directory then under that there is a juicebox_style.css file. The libraries.yml tries to load this with:

  css:
    theme:
      component:
        css/juicebox_styles.css: {}

In testing I got rid of this error message by editing settings.local to have:
$config['system.performance']['css']['preprocess'] = TRUE;
$config['system.performance']['js']['preprocess'] = TRUE;

where I previously had them set to FALSE. Check your settings.php and settings.local.php files for this.

I think the "qlg4wb" is a token that Drupal adds to the end of the file name. I was trying to track it down and understand it better when my settings.local changes made the error go away. Anyone who understands it better is welcome to add on here.

You might want to enable aggregation of css in your configuration/performance area too.

lpsolit’s picture

Ah, I see. "component:" shouldn't be there, AFAIK. It should be:

css:
theme:
css/juicebox_styles.css: {}

I did this change and the CSS file is loaded correctly and the error in the log is now gone.

fkelly12054@gmail.com’s picture

Thanks LpSolit. We will have to keep this in mind and get it into the next update of the package. After testing of course. I'm still not sure why Drupal adds those (what I call tokens) to the CSS files. There is a lot of stuff going on within the internals of Drupal to consolidate and compress the files and to assure that duplicates aren't loaded.

For the time being we have a work around for the error ... with your help and that of others we will try to make it go away before moving on.

neslee canil pinto’s picture

xem8vfdh’s picture

good work on this everyone, I may have to move back to Juicebox :)

Status: Fixed » Closed (fixed)

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