Module files can be loaded through the .info file in D7. It would be nice if we could extend this feature to library files. Fortunately, the API provides hook_registry_files_alter() that should allow us to do just that.

I have attached a patch that will allow modules to define whether or not their files should be autoloaded.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

elliotttf’s picture

Title: Allow modules to have their librarie files autoloaded » Allow modules to have their library files autoloaded
tstoeckler’s picture

Status: Needs review » Needs work

Hmmm, interesting patch.

I don't quite get what the purpose would be for modules to have their library files autoloaded. Autoloading only makes sense for PHP files, and only for those containing classes.

What I would imagine is for modules to declare that they depend on a library and we automatically load that library in libraries_init().

Or something like that. Haven't really thought the whole thing 100% through, but I'm not sure that your patch is the right way. Would love to hear you defend it, though :)

elliotttf’s picture

Hm, good point, I was going down the "bigger hammer" route without thinking too deeply about what I was doing.

Perhaps another option would be allowing individual files to be added to the registry, i.e.:

<?php
function hook_libraries_info() {
  $libraries['foo'] = array(
    'name' => 'Foo',
    'vendor url' => 'http://foo.com',
    'download url' => 'http://foo.com/download',
    'files' => array(
      'php' => array(
        array('file' => 'file.php', 'autoload' => TRUE),
      ),
    ),
  );
}
?>

Then in libraries_registry_files_alter() only add the files that have autoload set to true. There would obviously need to be a few other minor changes to check if the file was an array or a string (if we intended to to support both) but I think this method would be a good middle ground.

sun’s picture

#3 sounds better, although it's probably incompatible with our file definitions.

Overall, I wouldn't mind to simply register any kind of PHP file in the class registry. Most external PHP libraries contain classes anyway these days.

Crell’s picture

The issue with that, sun, is that some libraries have their own autoload logic that they may rely on, especially if they're using the SplObjectLoader approach. There is also the potential for class name collisions with the "blind parse everything" approach. In principle I agree that would be nice, but in practice I think it will be impractical.

sun’s picture

Thanks, @Crell! Your input is extremely valuable for these kind of issues :)

Alright, so right now, we have the following code for 'php' files:

  // Load PHP files.
  if (!empty($library['files']['php'])) {
    foreach ($library['files']['php'] as $file) {
      $file_path = DRUPAL_ROOT . '/' . $path . '/' . $file;
      if (file_exists($file_path)) {
        require_once $file_path;
        $count++;
      }
    }
  }

Considering that we'd be registering files containing classes to autoload via Drupal's class registry separately and those wouldn't have to be included manually, it might make sense to add a new/separate file group next to 'php' then?

Crell’s picture

Disclaimer: I don't actually know the guts of libraries API at all, so I'm speaking in general terms.

For OO PHP code (which is to be fair, the vast majority of it at this point), really all the libraries API needs to do is handle autoload registration. The actual loading of classes is something PHP itself can/should handle. The only catch there is whether we use our autoloader or one that the library provides / expects. I can't say for sure if any systems will actually break if they use our autoloader instead of their own. The most likely autoloader for them to use is the SplObjectLoader standard (aka Java-esque).

So... I suppose there's 3 possibilities:

1) Specific files should always be loaded when Drupal loads, period, because it's procedural code.

2) Specific files should be parsed by the registry, and then the autoloader takes it from there.

3) Some other autoload mechanism provided or specified by the library needs to be registered with PHP, and it takes it from there.

Make sense?

tstoeckler’s picture

Trying to translate this into Libraries API terms:

1. We need to differentiate between procedural PHP files, which we simply include upon loading and OO PHP files.

2. For OO PHP files we need some sort of 'autoload callback' which defaults to the drupal code registry (something like the path in the op) and can be set to something that uses SplObjectLoader. Not really sure how that would work, but I think it should be doable.

3. Does it make sense to register the files for auto-loading during the 'loading' phase of the library or do we need to make a special provision for that?

Crell’s picture

The autoload needs to be registered at Drupal startup. I suppose you could do it at library init, actually, but it would be really nice if you didn't even need to do that step and Drupal/PHP "just found it".

For the callback, hm. I don't know that it makes sense to register the actual autoload function itself. Rather, you want to register "something callable", which could be a function or a method of an object as in SplObjectLoader. You also do NOT want to register more than once. So if using the core registry, you would want to at install time have those files parsed and then forget about it.

I'm actually wondering if any Spl-structured libraries would indeed be incompatible with the registry. I'm not sure. The caveat is that they tend not to have easily enumerated files like our files[] array, so finding all of the files to parse may not be trivial.

tstoeckler’s picture

Over in #1023258: Make 'files' (and 'integration files') declarations consistent PHP files get converted from:
$library['files']['php'] = array('foo.php', 'bar.php');
to

 $library['files']['php'] = array(
  'foo.php' => array(),
  'bar.php' => array(),
); 

So I guess something like:

 $library['files']['php'] = array(
  'foo.php' => array('autoload' => 'SPLObject'),
  'bar.php' => array('autoload' => 'Drupal Registry'),
); 

(please don't mind the actual array values)
isn't too far away. And I think having these values, whatever they turn out to be, defined on file level (not on library level) is the only thing we can reasonably get away with.

One architectural shift this would bring with it, is that we would have to detect all libraries in hook_init(), but I guess that's not necessarily a bad thing.

sun’s picture

Dave Reid’s picture

Can we just keep this simple and re-use the autoload mechanism we already have? Let's just add this to a module's .info file:

library files[my-library][] = src/my-file.php   # Adds sites/all/libraries/my-library/src/my-file.php to the autoload file registry.
tstoeckler’s picture

That would be something like this.
Completely untested.
Pro:
* Easy
Cons:
* Non-pluggable autoload-mechanism
* Only works for module-provided libraries

sun’s picture

Status: Needs work » Needs review

Looks pretty good already.

That, combined with the idea of also plugging into hook_requirements() in #1215376: Autoload and populate drupal_get_filename() with libraries.module during Drupal installation could be a huge step forward.

However, I'm not sure what happens if multiple modules declare the same library files in their .info files. We'd have to test that.

RobLoach’s picture

Having #1321372: Load the files before stating the Library is loaded would allow modules to define how to load the Libraries via the "post-load" callback. This means that AutoLoaders could kick in like in Symfony.

....Are we talking about class autoloaders? Or are we talking about the ability to declare Libraries in a module's .info file?

Dave Reid’s picture

This is for class autoloading (aka core's file registry) so that after my module is installed I can just do $object = new MyLibaryClass(); without having to invoke any library.module APIs.

tstoeckler’s picture

Status: Needs review » Needs work

Here is an alternate approach. It is a little more verbose, but it is a little more flexible in that it also works for libraries provided by info files.

With this patch you could do:

// In hook_libraries_info() or in an info file:
$libraries['foo']['files']['php'] = array(
  'mySuperCoolClassLivesHere.php' => array('autoload' => TRUE),
);

// In your module, this should just work:
$bar = new mySuperCoolClass();

I didn't actually test this, this is more a proof-of-concept, so I'm setting to 'needs work'.

tstoeckler’s picture

tstoeckler’s picture

Oops, calling &drupal_static() correctly would be nice, but I hope you get the idea...

RobLoach’s picture

Very cool, I'm liking this....

+++ b/libraries.moduleundefined
@@ -9,10 +9,46 @@
 function libraries_flush_caches() {
+  registry_rebuild();
   return array('cache_libraries');

Does the Registry flush the cache for us when hook_flush_caches() is called?

+++ b/libraries.moduleundefined
@@ -9,10 +9,46 @@
+    if (($library = libraries_detect($name)) && $library['installed']) {
+      foreach ($files as $filename => $info) {

Do you know if this will only report files that it finds? Or does the registry protect against that for us?

+++ b/libraries.moduleundefined
@@ -9,10 +9,46 @@
+        $module = (isset($library['module']) ? $library['module'] : 'libraries');
+        $module_weight = db_query("SELECT weight FROM {system} WHERE type = 'module' and name = :name", array(':name' => $module))->fetchField();

Could probably get the module weight from system_list(). Also probably not good to do it in the foreach(), might want to move that out.

Also, might be handy to have some tests ;-) .

wizonesolutions’s picture

This looks like what I would need for amazon_fps, so if/when I port that to D7 I can test this patch if it still needs it at that time.

tstoeckler’s picture

Status: Needs work » Needs review

Moving to "needs review" for comments on the general approach here. I think this would be a really nice feature and the patch above (even though it's not 100% functional) shows it's not too hard. It just depends on how we want to it. So "needs review" doesn't mean nit-pick the code, in this case. Of course that also helps, though.

Status: Needs review » Needs work

The last submitted patch, 1092270-17-libraries-autoload-alternate.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review

Oh yeah, didn't realize that...
Well then setting to "needs review" again...

tstoeckler’s picture

Note that for 7.x-3.x we want to completely revamp Libraries API to use PSR-0 and Composer and everything. That will probably make this a lot easier.

donquixote’s picture

This is possible with xautoload-7.x-2.4.
#1781794: Registration of additional PSR-0 folders in modules and sites/all/libraries

function hook_libraries_info() {
  return array(
    'mymodule-test-lib' => array(
      'name' => 'My test library',
      'vendor url' => 'http://www.example.com',
      'download url' => 'http://github.com/example/my-php-api',
      'version' => '1.0',
      'xautoload' => function($api) {
        // Register a namespace with PSR-0 root in <library dir>/lib/
        // Note: $api already knows the library directory.
        // Note: We could omit the 'lib', as this is the default value.
        $api->namespaceRoot('XALib\TestNamespace', 'lib');
      },
    ),
  );
}
Chris Matthews’s picture

Issue summary: View changes
Status: Needs review » Closed (works as designed)
Chris Matthews’s picture

Status: Closed (works as designed) » Closed (won't fix)