Current "active" branch: 8.x-3.x-dev-1704738-tstoeckler

Copied from #1704734: [master] Libraries API 8.x-3.x:

Libraries 7.x-2.x already supported 3 types of libraries (JS, CSS, PHP). We want to

  1. Support more library types (Autoload, PEAR, ...)
  2. Allow certain libraries to have their own business logic with regards to loading, etc. One example would be supporting jQuery.noConflict() in order to allow loading multiple versions of jQuery on one page.

Both of these points can be easily achieved in an object-oriented fashion. Since (a) we gain automatic autoloading of the Library information files and (b) Drupal 8 is moving towards an object-oriented design, this seems like a natural fit. We will organize both our own files, as well as the Library classes in accordance with PSR-0.

Comments

tstoeckler’s picture

Priority: Normal » Critical

This is critical.

tstoeckler’s picture

Discussion item A:

We have the "libraries" namespace, so technically we should be doing LibrariesFooBar with our classes, which would mean LibrariesInterface and LibrariesBase. That is against our standards, and also sounds strange IMO, so I went with the more intuitive LibraryInterface and LibraryBase for now.

(Will post link to my branch soon.)

Edit: Named this discussion item A, so as not to lose track.

tstoeckler’s picture

In case someone is following along: Is our standard really to have a blank line after the beginning of a class definition, but no blank line before the ending? See http://drupal.org/node/1354#classes. I'm adhering to that for now, but that looks super weird. I might open an issue for that.

tstoeckler’s picture

Discussion item B:
Since not all libraries support variants, I think it makes sense to create a separate VariantLibraryInterface for those that do. That allows us to put code there (and in a corresponding VariantLibraryBase) that we don't load for libraries that don't need it, and for those that do we can define more stuff in the interface like variantLabel() (for choosing in the UI), etc. I went with that for now.

tstoeckler’s picture

Pushed some initial "code" (only interfaces for now) to 7.x-3.x-dev-1704738-tstoeckler
and added a link to that to the issue summary.

sun’s picture

It might make more sense to code against D8? i.e., 8.x-3.x?

However, we can also do 7.x-3.x, by adding a dependency on http://drupal.org/project/classloader

tstoeckler’s picture

I thought about that, too. For now I started with 7.x-3.x and already added a dependency on classloader.

I already had a use-case in mind, where we could use the Doctrine/Annotations, so maybe 8.x does make more sense. Given the expected release date of 8.x, I was sort of torn.

tstoeckler’s picture

Another short question, in case someone knows:
What is our standard for exceptions and namespaces?

I just wrote
Drupal\libraries\Library\Exception\LibraryVersionUndeterminedException
but that seems very bloated. I would rather do
Drupal\libraries\Library\Exception\VersionUndetermined
but I don't know what we do in these kinds of cases.

tstoeckler’s picture

Version: 7.x-3.x-dev » 8.x-3.x-dev

So I just created a 8.x-3.x branch in #1167496: Libraries API in core, which passes our existing tests against latest D8 core. Since the existing 7.x-3.x-dev-1704738-tstoeckler and 8.x-3.x-dev-1704738-tstoeckler branches are somewhat bastardized, as I mixed the porting effort and the actual rewrite, I will delete those and create a new and clean 8.x-3.x-dev-1704738-tstoeckler branch soon. There will be no 7.x-3.x-dev-1704738-tstoeckler anymore.

I know that we usually like to keep history, but in this case I think we can make an exception. Really not much happened in those branches yet.

I think we could also delete the 7.x-3.x branch, as that is basically obsolete for now, in order to save some maintenance effort, but it's not that much of a problem, and that would be somewhat of a bigger change, so I'll wait for some confirmation by @sun for that.

tstoeckler’s picture

Issue summary: View changes

Add link to current dev branch.

tstoeckler’s picture

I just rebooted the 8.x-3.x-dev-1704738-tstoeckler. For now I've kept it simple, and I only added code for now and did not touch existing code/tests.

Next step is LibraryManagerInterface and DefaultLibraryManager latter of which will find the library classes using Annotations.

Each library lives in a predictable namespace, because we force the machine-readable names to be identical to the class names*. So given the machine-readable library (class) name "Example" the namespace is "\Drupal\Library\Example".

So far, so good.

The problem arises because we want to support libraries in multiple possible directories, e.g. /, /sites/example.com/ (, /core/), ...**

I briefly chatted with @sun on Skype about that fact and we didn't really know how to solve that.

I thought that this is similar to how we register namespaces for modules, which can also live in different directories. The difference is we only have a single class per library, not a whole namespace. I.e. we want to avoid lib/Drupal/Library/jQuery/jQuery.php but instead we want lib/Drupal/Library/jQuery.php I re-read PSR-0 and it seems that it should be possible to directly register e.g. /sites/example.com/lib/Drupal/Library/jQuery.php as the "root" of the \Drupal\Library\jQuery namespace. I'm not 100% sure about that, though, and even less so whether the Symfony UniversalClassLoader actually supports that. I will find that out, though :-)

* This is already documented to a certain degree in the current code, but we should probably mention more explicitly that what we call "machine-name" here is not what we call "machine-name" in Drupal elsewhere.

** We should discuss where exactly inside the directories the library classes should live. We still want to put the actual library files in /libraries, i.e. sites/example.com/libraries. @sun said it would be best to put the library classes next to the actual library files, i.e. sites/example.com/libraries/Drupal/Library/jQuery.php I think more in-line with what we do in core and in modules would be sites/example.com/lib/Drupal/Library/jQuery.php This is what I am going with for now, but of course this can always be changed and we should discuss this in detail here.

tstoeckler’s picture

Started the Annotations stuff and opened #1781274: Make Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery more extendable. Depending on how fast that goes, I will postpone this on that.

tstoeckler’s picture

Closed #1781274: Make Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery more extendable, as it turned out to be dead-end.

I just committed the first version of an Annotations-based discovery mechanism, that solves the multiple-namespace-roots by avoiding it for now. :-) For now we simply search the given directories for class files and then let Annotations do its thing. We'll see.

The relevant code is:
http://drupalcode.org/project/libraries.git/blob/844d687fcf5d25b6031c2db...
It is completely untested so far, but could use some architectural review. Next step is converting libraries_test.module to provide some library classes and using that to test this.

tstoeckler’s picture

Status: Active » Needs review

I guess this deserves a "needs review" for now, of some sort.

tstoeckler’s picture

http://drupalcode.org/project/libraries.git/commit/b18ab479861d6b2d8c12b...
This commit fixes AnnotatedLibraryClassDiscovery to actually work. It's pretty cool. I also added some basic tests.

Regarding the whole namespace problem, I thought and read a bunch more about our proposal and it works! I can say so, because I actually tested it. :-)

Here's a short run-down of the problem-space and solution. This has been in my mind for a while, so I might be stating things as obvious, that are not actually obvious to anyone but me. Please follow-up in that case or if anything else below doesn't quite feel right to you. Here I go:

Requirement

We want to instantiate a library class given just the library machine name. So we need one central namespace hat holds all libraries, i.e. Drupal\Library\$MachineName. Because it looks nicer, and there wasn't a reason against it so far, I've chosen to make the library $MachineName camel-cased. We'll see how that works out. If we stick with it, we should document that more explicitly.

Problem 1

The problem is that we want to support a bunch of different directories for those library classes, i.e. \lib, \site\example.com\lib, \profiles\example\lib\, etc.

PHP supports defining namespaces in multiple locations, so we simply have to register all those possible paths to the Drupal\Library namespace. Problem 1 solved!

Problem 2

We also want to support overloading of library information, just like we do with module, i.e. you can have both /libraries/lib/Drupal/Library/Example.php and /sites/example.com/lib/Drupal/Library/Example.php. Now if they are in the same namespace, hilarity ensues...

...one would think. (At least I thought so.) Again, PHP comes to the rescue. If you register multiple paths for a single namespace PHP checks one after the other whether a certain class exists. So we only need to register the paths above in the right order and everything magically works. Problem 2 solved!

tstoeckler’s picture

Issue summary: View changes

Updated issue summary.

rjacobs’s picture

Issue summary: View changes

Housekeeping. I'm pretty sure this is now outdated and/or a dup of #2090623: Turn hardcoded library_load_files() into a more modern, flexible system?

tstoeckler’s picture

Status: Needs review » Fixed

Yeah, libraries are classes now, so in terms of the issue title, I think we can mark this fixed. We have #1704734: [master] Libraries API 8.x-3.x for keeping track of larger issues.

tstoeckler’s picture

Also removing the 8.x-3.x-dev-1704738-tstoeckler branch. That code is literally 4 years old and completely obsolete...

Status: Fixed » Closed (fixed)

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