Since Drupal 8 now has a very good class loader, which can easily be extended, This change will allow libraries to use the current class loader, instead of building a new one or using the class loader from the PSR0 library.

In the case of the Pheanstalk library in D7 I needed to add a post-load which would the supplied class loader and registed the library. Now all I need to do is specify the namespace and it is automatically loaded.

There is a minor change to the library data in the files section.

$library['exlib1'] = array(
  'name' => 'external library 1',
  'files' => array(
    'exlib1.php',
  ),
);

If this is a PSR0 library then you can do the following.

$library['exlib1'] = array(
  'name' => 'external library 1',
  'files' => array(
    'exlib1.php' => array(
      'namespace' => 'exlib1',
    ),
  ),
);

If the namespace is specified, then the included file is not loaded, but only used to check if the file exists.

CommentFileSizeAuthor
#3 libraries.code_.2068145-3.patch2.2 KBgordon
0001-Allow-use-of-Drupal-s-built-in-class-loader.patch2.13 KBgordon

Comments

Status: Needs review » Needs work

The last submitted patch, 0001-Allow-use-of-Drupal-s-built-in-class-loader.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Hmm... I'm not 100% sure yet, but I think this would be better suited as a new top-level key 'namespaces', which you could declare in the usual
$namespace => $path
way. And I guess we should support
$namesapce => array($path1, $path2)
as well.

See also #1092270: Allow modules to have their library files autoloaded for a previous discussion (D7, though).

gordon’s picture

gordon commented
Status: Needs work » Needs review
StatusFileSize
new libraries.code_.2068145-3.patch2.2 KB

This is mainly an update to the Use Composer's ClassLoader to handle class loading

I would argue against making namespace to top level entry as it is not consistent with the other top level entries. i.e. css, js, and php. If we look at css and js they have options and different methods of configuring them. No matter how me invoke the PHP code, either by including it or autoloading it it is still PHP. Adding the namespace below PHP means that all the PHP block is related to PHP code.

In the future we may add other types of loaders like a server side javascript using the V8js http://php.net/manual/en/book.v8js.php

Basically what I am saying is that the top level of the files is the language.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Yeah, I suppose you're correct. The code looks solid. There's one fundamental problem, I have, though, which was also discussed in the issue I linken in #2: If you want certain library files to be autoloadable, you shouldn't have to call libraries_load() at all. That's the whole point of auto-loading. Libraries API could parse all libraries for autoloadable files and register them at bootstrap time.

The problem then, however, is that there is no way to fail gracefully. With libraries_load() you can check whether anything happened before relying on the existance of the code. So we would need some facility for modules to declare their dependency on libraries. Which, in turn, is a whole can of worms, requires UI changes, etc, etc, etc....

So, in short: I'm not really sure what to do with this issue at this point.

gordon’s picture

gordon commented

Yes I thought about this too. We should not need to call library_load() if it is a PSR0 library.

But the scope of this patch is to reduce the memory footprint. So instead of using a libraries own autoloader (which most people will do) you can configure your library to be loaded by the composer autoloader.

I think that this patch also sets the format of the libraries_info() which will not need to be changed when we work out the best method to remove the need for library_load() for PSR0 libraries.

I think the removal of the libraries_load() from PSR0 libraries is beyond the scope of this issue, and we should move that is another issue. Worst case we don't work out how to address the issue of requiring libraries_load() but it does mean that people will be able to run Drupals builtin autoloader which will save on memory.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

I really hope libraries will take care of this feature.

Currently I have Graph API : #2168563: How to load graph and graph-uml php libraries? doing

function graphapi_loader() {
  $loader = drupal_classloader();
  $loader->add('Fhaculty', DRUPAL_ROOT . '/sites/all/libraries/graph/lib');
  $loader->add('Fhaculty', DRUPAL_ROOT . '/sites/all/libraries/graph-uml/src', TRUE);
}

which it bad.

But I really don't want the solution done by https://drupal.org/project/composer_manager require shell access and $ composer

Any thought?

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Issue summary: View changes
Related issues: +#2083547: PSR-4: Putting it all together, +#1992896: What recovery options do we have in case of RuntimeException.

Added XREFs

- WSOD for composer_manager #1992896: What recovery options do we have in case of RuntimeException.
- PSR-4 (as a reminder for this issue)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Closed (won't fix)

Modifying the class loader at runtime is problematic for a bunch of reasons and is obsoleted by proper Composer usage.

I could see some value in supporting D7's custom autoloader in Libraries API, but we already have #1092270: Allow modules to have their library files autoloaded to handle that.

Please re-open if you want to make a case for this feature, but I feel pretty strongly that this is "won't fix".