Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yhahn’s picture

FYI, this patch in the drush_make queue allows for libraries to be defined in your makefile and retrieved via the download method of choice.

http://drupal.org/node/583706#comment-2067524

mfer’s picture

From what I understand drush_make works to build new sites and not add stuff to existing sites. We need something to add stuff to existing sites as well.

joachim’s picture

I don't think you need drush make.
Use the same sort of stuff as the drush pm module -- that downloads files from a server, unpacks them, puts them in the right folders.

mfer’s picture

A couple questions....

Is the libraries api intended to hold the definition of where certain libraries files are to be download from? This would include an index of a lot of libraries? In D7 it will be easier to deal with because of hook_library().

Or, is the intent for the definition to live in the jquery_ui module and the libraries api just has the ability to download it for the jquery_ui module?

Or, both to some degree?

joachim’s picture

I would suggest it is up to the modules to say where they need things downloading from. More sense from an architecture point of view, and also means the module maintainer can specify the version that matches their code.

I can't find docs on how the API works so I can't make more concrete suggestions...

mfer’s picture

@joachim - http://api.drupal.org/api/function/hook_library/7

We would need to add a key for the files location which is fine. At the same time, you shouldn't have 2 versions of the same library on a site in most cases. It will just create namespace conflicts and other issues.

sun’s picture

@mfer: How. many. times. do. I. need. to. repeat. that. hook_library. is. NOT. the. same. but. a. totally. different. thing?

I also recommended to read up most of the details in the Libraries API issue linked on the project page, which should answer 90% of all questions. Or just look into the queue: #618370: Reasoning Behind Libraries API?

Is the libraries api intended to hold the definition of where certain libraries files are to be download from?

This is one of the remaining major issues that needs to be discussed (elsewhere). We face a N:N relationship here, where neither of both parties can reliably provide standardized library information. So the logical conclusion would seem to additionally allow a N:1:N relationship, which would mean that Libraries API would define a couple of libraries, but installation profiles as well as other modules would still be able to provide their own definitions. However, as soon as there is a 1:N relationship in terms of the library definition, we'll most likely have a problem. This particular question requires to understand the underlying, multi-dimensional challenge, which Libraries API is trying to solve.

tstoeckler’s picture

Status: Active » Needs work

http://drupal.org/cvs?commit=409402

Linking your work in progress here for potential discussion.

sun’s picture

Thanks! Seems I forgot to mention it here -- if it's not obvious already - that code was blatantly copied from Colorbox module; only minor adjustments, not working at all yet. ;)

tstoeckler’s picture

Title: drush support for downloading libraries » drush support for downloading and listing libraries
FileSize
44.3 KB
2.52 KB

I worked a bit on the listing part.
Attached is a patch and a screenshot of how it looks with the libraries_test.module enabled.

tstoeckler’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, libraries_drush_list.patch, failed testing.

tstoeckler’s picture

Now showing variants as well.

Patch also removes a variant from libraries_test.module, because that was clobbering the otherwise very nice table :)

Screenshot again attached (sorry, not on my TFT currently, that's why it's pretty small).

tstoeckler’s picture

Status: Needs work » Needs review

Let's see if the bot likes this one.

Status: Needs review » Needs work

The last submitted patch, 542940-libraries_drush-13.patch, failed testing.

sun’s picture

mmm, nice! :)

1) Name and title next to each other will be too wide for many shells. Can we either drop title or think of a way to output name below title?

2) The Version column should come after the Status column.

3) I don't understand the difference between "-" and "undetected" in version.

4) Can we shortcut "installed" with "OK" in the status column? We should make sure that there is a immediately visible difference between good and bad rows.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
45.95 KB
4.03 KB

Here we go.
Dropped title.
Made 'version' always show '-' in case of an error and 'status' show the error.
switched status and version.
'Installed' => 'OK'.

tstoeckler’s picture

Next round.
Status is now consistent:
'Not found'/'Not detected'/'Not supported'

sun’s picture

+++ libraries.drush.inc	10 Oct 2010 02:30:18 -0000
@@ -41,6 +41,65 @@ See libraries-list for a list of registe
+        switch ($library['error']) {
+         case t('%library could not be found.', array('%library' => $library['title'])):

UGH. :-|

Looks like we need to change 'error' into a constant (i.e., LIBRARIES_NOT_FOUND, etc) and move the error message into a separate 'error message' key.

+++ libraries.drush.inc	10 Oct 2010 02:30:18 -0000
@@ -41,6 +41,65 @@ See libraries-list for a list of registe
+           break;
+         case t('The version of %library could not be detected.', array('%library' => $library['title'])):

There should be a blank line between break + case lines, unless a case purposively falls through into the next case.

Powered by Dreditor.

Status: Needs review » Needs work

The last submitted patch, 542940-libraries_drush-18.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
45.9 KB
5.99 KB

Changed libraries_detect to not only the store the error messages (now $library['error message']) but also a short error "code" ($library['error']), which we directly display in the table.

Screenshot attached. (No visual changes, between #18 and this one).

tstoeckler’s picture

Now with less leftovers.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Yay, good job! :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 542940-libraries_drush-21.patch, failed testing.

tstoeckler’s picture

Committed #22 to CVS HEAD. Thanks for the great review session.
http://drupal.org/cvs?commit=434128

Marking 'needs work' for the harder part of this issue: libraries-download

tstoeckler’s picture

The 'error' -> 'error message' change broke tests. Committed the trivial attached patch to fix tests. The test bot doesn't like me nowadays, but I verified locally that there are exactly 4 fails before applying this patch, and applying fixes them.
http://drupal.org/cvs?commit=434230

tstoeckler’s picture

To actually allow downloading of libraries, we need to think about how Libraries API should access the files. Currently the $library['download url'] is for humans to visit and click on the right link, depending on the most recent version. So we need some automatism to do that.
Idea:
- Specify 'download url' like the following:
http://jquery.com/download/jquery.[VERSION].js
or similar.
- Either declare the latest version statically (bad), or introduce a 'latest version callback' that determines the latest version (hard)
- When downloading, do:

$download_url = $library['download_url'];
str_replace('[VERSION]', $version, $download_url);
sun’s picture

The latest known and supported version is the "largest" array key value in $library['versions'], no? version_compare() is able to figure that out.

EDIT: Also note that not all libraries use a simple pattern like "foo.[version].bar"... the URLs can be entirely different.

Additionally, we would have to set a (md5) hash to make sure that the downloaded library actually is the expected file. (Hackers everywhere, sadly.)

tstoeckler’s picture

1. Not all libraries have versions, but more importantly, because version_compare is so smart, the latest version might be 3.4.2, while versions only lists 3.

2. Right, so instead of having a 'latest version callback' we should have a 'url to download the latest version callback'?

3. Right. Doesn't make this any easier.

shiva7663’s picture

Is this an active plan? Drush support for the Libraries API in version 6 would be great.

fuzzy76’s picture

+1 and subscribing and all that...

RobLoach’s picture

Title: drush support for downloading and listing libraries » Drush support for downloading Libraries
Version: 6.x-1.x-dev » 7.x-2.x-dev
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
5.08 KB

Depends on Drush Make.

  $libraries['example'] = array(
    // Only used in administrative UI of Libraries API.
    'name' => 'Example library',
    'vendor url' => 'http://example.com',
    // Optional: A website link to the Library's download page.
    'download url' => 'http://example.com/download',
    // Optional: Information for automated downloading of the Library using
    // Drush Make. Visit Drush Make's documentation to read more regarding
    // the "download" property.
    'download' => array(
      // The type of download (file, git, svn, or cvs).
      'type' => 'git',
      // The URL or repository to download the Library.
      'url' => 'git://github.com/jane_doe/mytheme.git',
      // The revision information to check out.
      'revision' => '7.x-1.x',
    ),
tstoeckler’s picture

That indeed looks very good. I'm not too intimate with Drush Make, but I know that it supports downloading of archives as well, so it would be cool to give an example that does that.

+++ b/libraries.api.php
@@ -177,7 +177,19 @@ function hook_libraries_info() {
+      // The revision information to check out.
+      'revision' => '7.x-1.x',

As briefly discussed above, this seems to the crux of the issue: How do we find out which version to donwload? What this would mean is that everytime a new version of a library comes out, we would have to change the library information. Maybe as a first step, which would probably already help people a lot, we could pass the current version as an argument to the drush command. Then modules using Libraries API could document on their project page for instance: "Run drush libraries-download tinymce 4.3.1 for automatic installation of the library." Of course we would ideally like to find a way to automate all of that, but that would already be a huge step forward I think. Thoughts?

RobLoach’s picture

FileSize
8.24 KB

Here we go...

  • Added support for changing destination directory
  • Example of downloading a single file.
  • Support for overriding any of the download options (url, type, revision, branch, tag)
  • Ability to download all defined libraries
  • The version doesn't really make sense as part of the arguments as the revision might be a tag, branch, or revision option. So it's "--revision".

One question:

  1. Think this should be renamed drush libraries-make since it uses Drush Make?
    drush libraries-make tinymce
    
sun’s picture

- The empty $name to download all needs to be an explicit --all option. Wysiwyg and other modules are going to register many libraries ;) That said, not sure whether this makes sense at all; dropping it entirely might make more sense.

- As soon as this works with (tar) archives, I'd be happy to commit alpha-quality code.

- We need a way to declare whether the downloaded library archive extracts itself into a folder already or not. The folder possibly has to be renamed. Quite potentially, it's going to be easier to define whether extraction should skip a folder level (à la patch -p1) and declare the folder name to extract into (which we already know == library name).

- Revision might make sense for version controlled, checked out repositories, but perhaps not for releases. We might need an additional release identifier option.

- Speaking of, can we support something like drush pm à la tinymce-3.3, i.e., [libraryname]-[version]?

- The download URL often contains the version number. However, we could as well limit this feature to libraries providing a "latest stable" download URL only.

RobLoach’s picture

The empty $name to download all needs to be an explicit --all option. Wysiwyg and other modules are going to register many libraries ;) That said, not sure whether this makes sense at all; dropping it entirely might make more sense.

I like your --all idea, but I think we should just drop the support for that and push it to a separate issue.

As soon as this works with (tar) archives, I'd be happy to commit alpha-quality code.

#622224: add support for .tar files ;-)

We need a way to declare whether the downloaded library archive extracts itself into a folder already or not. The folder possibly has to be renamed. Quite potentially, it's going to be easier to define whether extraction should skip a folder level (à la patch -p1) and declare the folder name to extract into (which we already know == library name).

Drush Make handles that and renames the extracted folder to the library name. Probably seems like something we should push over to the Drush Make issue queue?

Revision might make sense for version controlled, checked out repositories, but perhaps not for releases. We might need an additional release identifier option.

It definitely depends on which type of download you're getting. You don't need the revision option when you're downloading a "file" type. Maybe this just means that the module developer needs to be more cautious with the version callback and variants? It should be up to the maintainer as to how that's handled.

Speaking of, can we support something like drush pm à la tinymce-3.3, i.e., [libraryname]-[version]?

It depends on how the library is downloaded. If TinyMCE is handled by just a file download, then drush libraries-download tinymce --tag="3.3" does not make sense. If it's handled via GIT or SVN though, then that's a valid call. Again, completely depends on the type of download.

The download URL often contains the version number. However, we could as well limit this feature to libraries providing a "latest stable" download URL only.

Maybe extend support for the "versions" array to allow the "download" options?.... Then we could have: drush libraries-download example --version="3.0" and it uses the download options from that version if it's defined?

sun’s picture

I don't really get why .tar archive support is a Drush Make issue and not a Drush Core issue, but who am I...

Maybe this just means that the module developer needs to be more cautious with the version callback and variants? It should be up to the maintainer as to how that's handled.

Version callbacks and stuff are only invoked after the fact. This code here runs before the fact.

Libraries API's design idea is that integration code within Drupal mainly or ideally cares for major versions of a library only. As such, a download facility would ideally download the latest version of a library, if provided by the library vendor. After downloading, the Drupal/Libraries integration code determines the library's version and primarily differs per major version. In an ideal world, integration code is compatible with libFooBar version 3.00 to 3.99, and it only needs to change if and when a 4.x comes out.

Long story short, we don't know which exact version to download upfront, because anything < 4.x should work. If the vendor provides a static latest stable release download URL, then we're good to go.

However, we definitely don't want to hard-code any library version or revision to download. So let's simply leave that out entirely.

RobLoach’s picture

I don't really get why .tar archive support is a Drush Make issue and not a Drush Core issue, but who am I...

There are talks to fix that: #1310130: Put drush make in drush core ..... Hmm, #1267228: Drush Make should use Drush core's native download abilities concurrently.

RobLoach’s picture

FileSize
9.48 KB

Did some work on this....

  • Support for both Drush Make 4.x and Drush 5.x (see #1310130: Put drush make in drush core)
  • Renamed to "drush libraries-make" as it pretty much is Drush Make support for Libraries
  • Now has a drush libraries-make --all option
  • Cleaned up the code a bit

Would be nice to have these patches in Drush:

RobLoach’s picture

  1. Should we change the "download" key to "make"? $libraries['tinymce']['make']['url'] = 'asdffsda.tar.gz';
  2. We should have an alias for "libraries-list" as "libraries". drush libraries
RobLoach’s picture

Status: Needs review » Postponed

This issue is kind of required for this to go in before Libraries API can have it: #1369922: Fix up the Return values of Make Download

Otherwise, Libraries API always returns a failure when downloading libraries, even though it succeeds.

RobLoach’s picture

tstoeckler’s picture

First off, thanks again for starting this, I really think this will be a game-changer for our users!

I finally got around to fiddling with this for a bit, and in trying to grasp it and make it work for a few example libraries, I changed this and that, and in the end I deviated enough from your patch, to roll a new version (after cleaning up after me, of course).
The patch might look pretty finished, or "full-fledged" (docs, etc...), but it's really just my proposal of how I would do it, what I think are the most usual use-cases, etc.
So if it is the case, "Your patch sucks, #39 is better!" is very much a possible response to this. :)

That said...
...this patch makes it so that you can/have to specify the version you want to download when you are downloading it, i.e.:
drush libraries-make jquery 1.7.1
I think that is more in-line with our goal to not have to adapt our release-cycle(s) to that of any libraries.
It enables that by allowing the use of a "!version" placeholder in the download options, for example:
$library['download']['url'] = 'http://code.jquery.com/jquery-!version.js';
(Yes, there's also a !variant placeholder.)
When downloading from a Git repository, you can specify
$library['download']['tag'] = '!version';
And then the "1.7.1" tag will be checked out upon download.

NOTE: I used this as my primary resource for Drush Make (and also documented it in-code). It only lists the 'tag' parameter (and 'branch', etc.) for git, not for bzr or svn. So for now I only documented the whole thing for 'git'.

With this patch I was successfully able to download:

  • a static file
  • a git repository
  • a zip archive
  • a tar archive

For convenience, I've attached the .libraries.info files I used.

NOTE2: jQuery does
http://code.jquery.com/jquery-1.7.1.min.js vs.
http://code.jquery.com/jquery-1.7.1.js
so you can't really specify the !variant placeholder as sometimes it's there and sometimes it's not. For that reason I've allowed the same kind of version- and variant-overloading we do elsewhere for this.
Because I was lazy, I then also utilized that for the different download methods for jQuery UI.
See the info files, for what I mean in detail.

The commands are:

# static file
drush libraries-make jquery 1.7.1
# extra credit
drush libraries-make jquery 1.7.1 --variant="min"
# git
drush libraries-make jquery-ui 1.8.16
# zip
drush libraries-make jquery-ui 1.8.16 --variant="zip"
# tar
drush libraries-make jquery-ui 1.8.16 --variant="tar"

Have fun! :)

NOTE3: I changed how the drush command detects success/failure, because you're version wasn't working with my version of Drush (4.5), but I think you a lot more into Drush than I am, so I would defer that to you. The way it is in this patch is only because that worked for me.

RobLoach’s picture

+++ b/libraries.api.phpundefined
@@ -329,6 +385,11 @@ function hook_libraries_info() {
+    'download' => array(
+      'type' => 'git',
+      'url' => 'http://github.com/tinymce/tinymce.git',
+      'tag' => '!version',

Could modules provide a default version number?

+++ b/libraries.drush.incundefined
@@ -88,64 +102,125 @@ function libraries_drush_list() {
+  $destination = drush_get_option('destination', 'sites/all/libraries');
+  $destination = DRUPAL_ROOT . '/' . $destination . '/' . $name;
+
+  // Create the directory.
+  drush_mkdir($destination);
 
-  // Set working directory back to the previous working directory.

Thanks so much for testing this out and pushing it forward! Although I won't get a chance to try it until tomorrow, I thought I'd point at #1143936: Downloading a single file breaks if the folder doesn't exist regarding jQuery and failing on single file downloads.

+++ b/libraries.drush.incundefined
@@ -88,64 +102,125 @@ function libraries_drush_list() {
+ * @see libraries_invoke()
+ */
+function libraries_prepare_make_download(&$library, $version = NULL, $variant = NULL) {
+  if (!empty($library['download'])) {
+    // We support the !version and !variant placeholders for all of these values.
+    foreach (array('url', 'filename', 'tag') as $key) {
+      if (isset($library['download'][$key])) {
+        $value = $library['download'][$key];
+        // If one of the placeholders was used in the value, but was not specified,
+        // set an error.
+        if (strpos($value, '!version') && empty($library['version_to_download'])) {

Unfortunately this hook isn't called if installing a distribution from the .make file via libraries[]. But I suppose a .make file would actually provide the straight up version number and not the "!version" ;-).

+++ b/libraries.drush.incundefined
@@ -88,64 +102,125 @@ function libraries_drush_list() {
+
+  // Create the directory.

Thanks so much for testing this out and pushing it forward! Although I won't get a chance to try it until tomorrow, I thought I'd point at #1143936: Downloading a single file breaks if the folder doesn't exist regarding jQuery and failing on single file downloads.

tstoeckler’s picture

1.

Could modules provide a default version number?

Hmm... what is the use-case for that?
What I'm trying to avoid is anything that has to be updated in our code as soon as library X decides to roll a new release.

Thinking about it, though, that would allow
drush libraries-make jquery
instead of
drush libraries-make jquery 1.7.1
though, which is very appealing, obviously.

Hmm.... :) Not sure.

2.
I wasn't aware of #1143936: Downloading a single file breaks if the folder doesn't exist, thanks.
That's exactly what I added that mkdir() for, and it felt pretty hacky.
So I guess we can put this postponed on that. It's pretty minor, though so I guess we can still flesh this out in the meantime.

3.

Unfortunately this hook isn't called if installing a distribution from the .make file via libraries[].

Hmm... make files will simply call drush make and not drush libraries-make so you have to specify the whole information yourself anyway.
In the long run, of course, it would be nice, if we could have some sort of automation, instead of having to duplicate that information, but I can't even start to wrap my hand around that right now :)

sun’s picture

There should be no version numbers anywhere, neither in the library base definition nor on the drush command line.

Libraries API is always passive. Users smack an arbitrary library on it and it is supposed to figure out which library that is and which version. API consumers then react to that knowledge.

The only place where versions of a library are contained is in the extended library definition. However, those versions act as "minimum version and above", or in other words, the "last known version[s] that we know how to deal with."

This means that the library definition and most often also integration code can stay the same until there is an API change in the upstream library. Only if that happens, the library definition needs to be updated.

Users should always download the latest version of a library.

For this reason, there should never be any hard-coded version numbers anywhere in Libraries API and library definitions.

--
This is key to understand. Would be beneficial to document this in the handbook, or mayhaps even in the code docs.

Thinking through this, I really think that we should only support downloading of libraries, which provide a stable download URL. Or in precise other words:

Unsupported: http://example.com/lib/release/example-1.23.tgz

Supported: http://example.com/lib/download/example-latest-stable.tgz

If a library does not support this, then downloading is not supported. People need to ask the vendor to provide it. Possible in multiple ways, most simple being a symlink to an existing, version-specific file like the former.

tstoeckler’s picture

I totally agree, which is why I wrote the patch in that way :)

Users should always download the latest version of a library.

...they just have to quickly look up, which one that is. Nothing is hardcoded.

In short: Your reply seemed to be critical of my approach, I just didn't really get in which way :)
It'd be awesome if you could clarify how the user-interaction would work in your perfect world.

I really think that we should only support downloading of libraries, which provide a stable download URL.

I thought so too, at first, until I was unable to find such a URL for jQuery even.
So maybe, that's unrealistic. Not sure. (I also didn't look for too long, so maybe it's me...)

RobLoach’s picture

I definitely like the direction of this. It would just be nice to say "install this library", and then it just gets it, without having to worry about the version number.

Unsupported: http://example.com/lib/release/example-1.23.tgz

Supported: http://example.com/lib/download/example-latest-stable.tgz

If a library does not support this, then downloading is not supported. People need to ask the vendor to provide it. Possible in multiple ways, most simple being a symlink to an existing, version-specific file like the former.

As much as I would love for this to happen, most projects don't actually support this. Symfony has a pretty complex build system, and it might actually be a good test-bed to try this stuff out. I stuck the download options into the Symfony module, as well as the option to build from the sources via drush symfony-download. But, it doesn't support downloading the latest stable build.

Maybe this is where Packagist and Composer come in to handle that stuff? Either way, I'd say the latest patch is the way to go.

RobLoach’s picture

http://drupal.org/project/composer

drush dl composer
drush dl symfony
tstoeckler’s picture

Title: Drush support for downloading Libraries » [meta] Support for downloading libraries via Composer
Priority: Major » Critical
Issue tags: +libraries-7.x-3.x

@sun and I recently had a nice chat about the future of Libraries API, and this issue is a central piece of that.

In short, the idea is to create a central repository of library information (see #773508: [meta] Provide a central repository of library information (i.e. libraries.drupal.org)), which also stores a composer.json file for each library. Libraries API would then retrieve that composer.json file and download the library using that.

This is critical for 7.x-3.x.

tstoeckler’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev
Status: Needs review » Active
Issue tags: -libraries-7.x-3.x

Tagging with the new version, now that we have a 7.x-3.x branch.

Also marking "active", since we will have to probably start from scratch here.

tstoeckler’s picture

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

Changing version to 8.x-3.x

donquixote’s picture

Looking at composer, it seems that really a lot of what is discussed here happens automatically.

    "require": {
        "php": ">=5.3.0",
        "guzzle/guzzle": ">=2.0.0",
        "Monolog/monolog": "1.0.*"
    },

This is the composer.json from one project I looked at. The json does not declare any download locations. It just says name and version number. Composer looks into packagist or any repository, to find the download location. For the start, we should just rely on packagist, and not care about other download locations.

Libraries that are not in packagist, or not supported by composer at all, can still be downloaded the manual way.

--------

The first idea would be:

function hook_libraries_info() {
  $info['monolog'] = array(
    'composer' => array('Monolog/monolog', '1.0.*'),
  );
  return $info;
}

Problems:
- What if more than one module adds this library? The usual module_invoke_all() will overwrite the version info.

------------------------

Next idea: composer.json file per module.
Problem: That's too much! We only need the "require" part.

--------------

Next idea: Key in the info file.

name = My module
; Yes, we want this to work also for 7.x, not just 8.x
core = 7.x
dependencies[] = libraries
composer[Monolog/monolog] = 1.0.*
; or
composer[] = guzzle/guzzle >= 2.0.0

The nice thing is, this would seamlessly integrate with Drupal 7.
Libraries can use hook_system_info_alter() to somehow block the module from being installed, while the library is missing. Or do this in some other way.

Composer module (the git extension) can have a command
drush composer-install-all
to learn about missing libraries from system info, download them with composer, and store them in a vendor directory.
(that we still have to agree about)

Why libraries module and composer module?
Simple:
Composer is for the drush command, and is not installed on-site as a regular Drupal module.
Libraries is a regular Drupal module, and thus able to prevent modules from being installed, if their libraries are not present.

------------

What about class loading?
There are different ways to do this, and it would be great to have choice:
- libraries can simply include the autoload.php file generated by composer. Done. Yes, it should be libraries module which does that, because composer module is not available at request time.
- classloader module could register the namespaces found in the composer-generated autoload_namespaces.php
- xautoload module could do the same.

Imo, libraries should simply make sure that one of those solutions is in place.

-------------------

Location of the composer vendor directory?
I'd say we should not mix that with sites/all/libraries. Why? There can be things in sites/all/libraries which totally contradict composer.
So, I suggest
- sites/all/composer
- sites/all/vendor
- sites/all/composer/vendor
Is there anything in D8 already? It could also be a different location in D8, because sites/all/ is no longer the "place for everything".

See also #1433256: Proposal: Composer as a Library Manager

----------------

What about composer dependencies in libraries downloaded manually?
E.g. your custom library downloaded from github contains a composer.json. However, that library itself is not in composer.
Solution:
- Modules can declare in hook_libraries_info(), that a given library contains a composer.json. This is to prevent ridiculously deep file scans, or false positives caused by in-development files.
- Composer / libraries use this info to refuse module enabling, or to download those additional libraries.

donquixote’s picture

Addition to #53:
I think it would be better to rename the key in the info file:

name = My module
description = A module that needs a composer-provided library
core = 7.x
dependencies[] = libraries
composer.require[] = guzzle/guzzle >= 2.0.0

And, damn, now we also want to require a specific version for libraries module! Is that even possible?

[crazy idea]
What about this? (hacky, hacky, hacky)

name = My module
description = A module that needs a composer-provided library
core = 7.x
dependencies[] = libraries
dependencies[] = composer:guzzle/guzzle >= 2.0.0

This will explode, until you install libraries-7.x-3.x, which will use hook_system_info_alter() to clean up the dependencies[].
Problem: It can also explode in other modules' implementation of hook_system_info_alter(). So better not..
[/crazy idea]

donquixote’s picture

Rethinking this:
Actually there is no reason why this all has to be parts of libraries module.

There are several reasons to make this a separate module:
1. There is very little in current libraries module, which this composer stuff could build on.
2. Composer libraries should live outside of sites/all/libraries (imo)
3. It is easier to specify a dependency for a new module (e.g. "composer_libraries") than for a specific version of libraries module.
4. The new module can have its own issue queue, versioning etc.
5. Libraries can continue to do the job for non-composer libraries.

The little problem: The most logical name for this module ("composer") is already taken by a drush extension. What we need is a regular module which can be installed on-site. I don't know whether it is feasible to turn composer from a drush extension project to a regular module, and what would be the implications of that.

RobLoach’s picture

Caution: My arguments may be a bit skewed ;-) ... Reading through the comments, it sounds like you guys are looking for http://drupal.org/project/composer_autoload .

Libraries that are not in packagist, or not supported by composer at all, can still be downloaded the manual way.

Actually, you can declare your own packages that aren't on Packagist in the repositories key.

Next idea: composer.json file per module.
Problem: That's too much! We only need the "require" part.

Not entirely, although PEAR is still a thing, Composer has almost become the defacto-standard for maintaining and defining PHP packages. It comes with many features that would be silly to ignore:

  • Generation of a static class-map so that PSR-0 lookups arn't even needed (composer install --optimize)
  • Allows use of "provide" to allow packages to ask for implementations of a certain interfaces. Like a module asking for an implementation of a Plugin without having to worry about what that Plugin is.
  • Already saturated community with at least 4500 packages

Composer module (the git extension) can have a command
drush composer-install-all

Was playing around with that idea: #1452984: drush composer-mass install ... Would be nice to just download all your modules, run a composer-install-all and have everything work would be nice. Currently, it's executed on the drush dl hook, but tighter integration would be great.

Composer is for the drush command, and is not installed on-site as a regular Drupal module.

Correct. Bootstrapping the vendor code in your module means loading autoload.php. There is the Composer Autoload module that'll do that for you though.

Location of the composer vendor directory? sites/all/composer - sites/all/vendor - sites/all/composer/vendor

Vendor code location is handled by autoload.php, so we should not worry about that. composer.json's vendor-dir in config is responsible for that. All we need to do is load autoload.php.

Is there anything in D8 already? It could also be a different location in D8, because sites/all/ is no longer the "place for everything".

It's in /core/vendor in Drupal 8.

What about class loading? libraries, classloader module, xautoload. Imo, libraries should simply make sure that one of those solutions is in place.

Libraries, XAutoload, Classloader, Composer Autoload, they all accomplish different things. Although they're hitting similar goals, they all accomplish them differently. Whether a module wants to use one either the other should be up to the module maintainer. XAutoload and Classloader seem to be the most similar. XAutoloader does provide a bit more flexibility than Classloader though.

Where Libraries differs from them all is that it can be used to load JavaScript/CSS vendor code. Been playing around with the idea of having a Require.js config written for JavaScript/CSS components from Composer (think a autoload.js file): https://github.com/robloach/components .... But that's FAR off, and I'm not even sure that's the correct direction for JavaScript/CSS assets, but it is something to play with. This would seem like a PHP-implementation of Component, JamJS, Bower, or any of the other web browser package managers out there.

Sticking composer.json information in module .info files

This seems very backwards. Composer gets its information from composer.json. Although it's somewhat confusing, a Drupal module dependency is different than a PHP package dependency. I outlined why that is over at How a .info file would be translated to a composer.json file.

donquixote’s picture

Actually, you can declare your own packages that aren't on Packagist in the repositories key.

This is cool.
So, we have the choice between
- composer.json per module, with "require" and "repositories".
- module info file, with "composer.require[] = .." and "composer.repositories[] = .."

That's a typical DX bikeshed :) I have my own preference (more on that below), but it is not really a blocker for anything. Both should work ok technically, if the only purpose is to know which composer libraries are needed.

Composer has almost become the defacto-standard for maintaining and defining PHP packages. It comes with many features that would be silly to ignore

Drupal modules are far away from being "PHP packages". Thy are typically designed to only just work in Drupal, and nowhere else. And they are hosted on drupal.org, (except for custom modules).

This doesn't have to stay this way forever, btw. It is attractive to think about Drupal modules shipped with composer-powered PHP packages. But that's a long way, and requires more thinking than just a composer.json put in the module dir. As long as modules are just modules, a per-module composer.json will confuse more than it helps (imo).

Currently, if you want more of your Drupal module reusable outside of Drupal, the solution is probably to
- have the reusable stuff in a composer repository
- have the drupal-specific stuff hosted on drupal.org, with a composer dependency

Generation of a static class-map so that PSR-0 lookups arn't even needed (composer install --optimize)

I doubt the performance benefit of that in a Drupal context, if compared to xautoload (with or without APC cache).
It might be useful for classes that are used a lot, or for projects that don't have a lot of classes.
And of course, if your PSR-0 implementation does not scale to a high number of namespaces.
Some benchmarks could be nice.

Allows use of "provide" to allow packages to ask for implementations of a certain interfaces. Like a module asking for an implementation of a Plugin without having to worry about what that Plugin is.

"provide" is cool.
But Drupal modules are not composer packages.
- composer packages (and even custom-downloaded libraries) can provide a composer.json with "provide".
- Drupal modules should not "provide" anything, because composer does not treat them as packages.

-------------------

Ok, before I continue:
I think the major question is: Should we treat Drupal modules as composer packages? In D7? What would be necessary to do that, what would be the implications?
Or can composer packages include Drupal modules?
I think this is all very sexy, but goes far beyond the problems we want to solve in this issue: Make composer libraries available to Drupal, and let modules declare which libraries they need.

Let's clear this up before we get lost in more detail.

donquixote’s picture

Can't help it, have to comment more :)

Location of the composer vendor directory? sites/all/composer - sites/all/vendor - sites/all/composer/vendor

Vendor code location is handled by autoload.php, so we should not worry about that. composer.json's vendor-dir in config is responsible for that. All we need to do is load autoload.php.

[..]

It's in /core/vendor in Drupal 8.

This does not solve any problem.
What we want is one single place within a Drupal install for composer libraries.
/core/vendor is not an option, because that's for stuff shipped with core.
So it has to be sth like sites/all/composer, or sites/all/composer/vendor for D7, and something else for D8.

"composer.json's vendor-dir in config" would only help if we have one global composer.json for the Drupal install.
This is not going to happen. Or if it happens, then it is going to be generated by composer or libraries or whatever, and then we still need to make that decision deliberately.

Having modules negotiate where they want their composer vendor directories is asking for trouble.
The module dir itself is not an option, because this is wiped on update, or would make a mess in git. Also, libraries would no longer be shared if we have a per-module vendor dir.

donquixote’s picture

Ok, here is an updated proposal:

- Modules provide their composer dependencies in one way or another. Modules themselves are not treated as composer packages.
- drush composer creates a sites/all/composer/composer.json (D7) or just composer/composer.json (D8), with all dependencies assembled.
- drush composer does the usual composer recursive downloading, starting with that generated composer.json. Vendor directory will be sites/all/composer/vendor/, or just composer/vendor/

donquixote’s picture

Re #57:
This all refers to what modules are in D7.
If #1398772: Replace .info.yml with composer.json for extensions and other changes land in D8, then some of my arguments might no longer apply - but should still be considered for the D7 version.

boombatower’s picture

My understanding when talking about adding composer files to modules and such was not that they would be considered packages that can be used out of drupal, since in fact the type can be set to drupal-module which is clearly drupal specific. The benefit is that we do not have to a) maintain a drupal specific package management system (drush make), b) easier for people to jump into drupal since composer is more widespread, c) bonus features and future improvements from composer.

I have given substantial thought to what is necessary to replace drush make with composer which it sounds like this conversation is on the verge of working through.

It seems rather silly to make a distinction between modules and other code which are all reusable units and have the same management need, just that modules are drupal specific (not sure if everyone is aware of https://github.com/composer/installers/blob/master/src/Composer/Installe... and the related code in there). Meaning that modules would remain drupal specific just managed by composer instead of drush make which makes a lot of sense.

What is the reasoning for "forking" the composer.json file into .info files? This seems like a needlessly introduced Drupalism. If anything we should be looking at doing the reverse since composer files contain more information than just dependencies (like title and description) and are only missing a few drupalisms. Keeping them separate also makes sense, but dumping composer.json into .info file seems counter intuitive.

donquixote’s picture

#61
- a module can not be downloaded separately (only the "project" can).
- a package can not be "enabled".

If composer gets to see a package with a number of modules, and some of those modules have dependencies, then how can composer know which of these are enabled? Because we don't need to download dependencies for disabled modules.

boombatower’s picture

The issue about disabled modules is an issue with drush make currently and would have to be code that sits on top..or people splitting out the modules within their projects into more logical units, but both of those problems are not composer specific and exist now. In fact they exist in the composer work...you simply download all dependencies regardless of whether you use the code.

- a module can not be downloaded separately (only the "project" can).

Sure that makes sense, I tend to think of module as a project since more are or you depend on the primary one anyhow, but yes they are separate...again this problem has not been solved by drush make either so we are just switching packaging systems and this needs to be solved separately. It all boils down to the fact that projects should be separated if they are to be downloaded separately.

donquixote’s picture

Are we still talking about contrib solutions here, that also work for D7? Or should we continue in the core issue?
#1398772: Replace .info.yml with composer.json for extensions

I get the idea, whatever D8 does with composer, should rather live in core. Especially, if we want to use that for module downloading.
For D7, we should find a conservative solution in contrib, we cannot just reinvent the info file and replace it with something else.

Also I consider our problem scope for D7 very limited: Find a solution to automatically download 3rd party stuff with composer, based on dependency information found in modules.

Or maybe I am on the wrong issue, because this is tagged 8.x-3.x-dev.

Sure that makes sense, I tend to think of module as a project since more are or you depend on the primary one anyhow, but yes they are separate...again this problem has not been solved by drush make either so we are just switching packaging systems and this needs to be solved separately.

Ok let's assume we replace drush make with composer. What does this change?
"Projects" become "packages", and can have a composer.json if you like.
Modules are still modules. We can have more than one per package, they can have dependencies to other modules and to packages, and we need to know which of them are enabled.
Packages can contain some code that is always available once the package is downloaded, and other code that is only available if a specific module within the package is enabled.

Also consider that modules in D8 are also in some way Symfony "bundles" (or they contain them, or whatever), so this adds a 3rd level to the problem: Package vs module vs bundle. And bundles have yml files..

cpliakas’s picture

Just to make sure everything is cross referenced, there are some similar discussions happening in the Composer Manager issue queue.

tstoeckler’s picture

Issue summary: View changes
Status: Active » Closed (won't fix)

Now that Drupal has properly embraced Composer this is no longer necessary. And in fact it's not possible to properly download Composer dependencies from a module.

We will need some way to download asset libraries at some point in D8 similar to what we have in D7 now with drush libraries-download, but we can fix that in a separate issue.