Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DX] Standardize on a place for shared libraries to be used by multiple modules. #159

Open
9 tasks
klonos opened this issue Jan 10, 2014 · 39 comments
Open
9 tasks

Comments

@klonos
Copy link
Member

klonos commented Jan 10, 2014

I propose we go with /core/libraries/ for core libraries and /libraries/ for contrib.

We should also adapt backdrop_get_path() to work for libraries as well as modules, themes, and layouts. This is the most useful part of the Libraries API project, and it would be good to get that into core. Here are the usage stats (more than half a mil!!!) on that module.

Proposed solution:

  • Add library detection support to backdrop_get_path(). So you could use backdrop_get_path('library', 'fontawesome') to locate a directory called fontawesome in any of the following locations.
    • core/libraries
    • libraries
    • sites/[site_name]/libraries
  • The backdrop_get_path() for libraries would follow the same selection rules as for modules/themes/layouts. They could be overridden by using more-specific directories (/libraries overrides /core/libraries, etc.)
  • Unlike other top-level concepts, libraries by their very nature would not include a .info file. But they would still be rebuilt and stored in the system table like modules and themes (I think, need to evaluate this).
  • It would be at each module's discretion whether they wanted to bundle libraries directly or use backdrop_get_path() to locate a library. In any case, they would still need to implement the current hook_library_info() hook. But this hook may (or may not) use the generic library path, the bundled, path, or conditionally the generic path if it exists or the bundled path otherwise.
  • We add an additional "version callback" property to hook_library_info() to match Libraries API's ability to determine version numbers and verify that library paths are correct.
  • System module would get a new check in hook_requirements() to get library info from all modules. Those modules that declare libraries that include "version callback" properties would be verified that the library was in the right location and could be read properly.
  • Core would provide a few generic version callbacks, such as library_read_composer_version(), library_read_bower_version(), and library_read_package_version(). These could be reused by contrib for handling libraries that support Composer, Bower, NPM, or whatever package manager comes out next month.
  • Version callback information would be stored in the system table to save processing. We'd need to add a new function similar to system_rebuild_module_data() that rebuilt library data on cache clears.
  • Version dependency could be implemented in the first implementation of this functionality, or left for later and in the mean time implemented through individual module "version callback" implementations that combined version checking with the version parsing.

This change will need a change record created on api.backdropcms.org

@klonos klonos changed the title Include Libraries as a core module Include Libraries API as a core module Jul 29, 2014
@klonos klonos mentioned this issue Oct 9, 2014
34 tasks
@quicksketch
Copy link
Member

I agree we need some kind of library management. Libraries API is a bit bloated (all I ever use is the library_get_path() and maybe the version checking), but I think it's important that we standardize on a place for shared libraries that are used by multiple modules. Even core would benefit from having a better place than core/misc to include all its libraries. I'm not sure this would need to be a separate module. In core, it would probably be bundled into common.inc or system.module.

@klonos klonos changed the title Include Libraries API as a core module Standardize on a place for shared libraries to be used by multiple modules. Oct 10, 2014
@klonos
Copy link
Member Author

klonos commented Oct 10, 2014

Yes, I don't care about the module itself or the implementation. All I'm interested in is a single place to put 3rd party libraries and the ability to upgrade/downgrade them at will without having to wait for a new core version to be released and include them (eliminate the need for modules like jQuery Update for example).

@klonos
Copy link
Member Author

klonos commented Oct 10, 2014

...auto-discovery and enabling/disabling them or even switching between multiple available versions in a "Libraries" admin page (like the modules page) would be perfect.

@ghost
Copy link

ghost commented Oct 10, 2014

Is this for core libraries, contrib libraries, or both? @klonos' comment about eliminating the need for jQuery Update seems to suggest being able to replace/upgrade jQuery in core, presumably without 'hacking' core...

@klonos
Copy link
Member Author

klonos commented Oct 10, 2014

I guess we can have /core/libraries/library_name/library_version for the ones that we ship officially with core, and keep /libraries/library_name/library_version for contrib and themes to use.

We can start by giving the /libraries folder priority over /core/libraries for easy replacement and then add an admin page to allow site builders to select from all the available versions to use from the UI.

I know that we can go without having a library_version subfolder, but it would make it easy to have multiple versions in a setup (with only one "active" at each given time) and this in turn is very useful for troubleshooting by switching versions (that's how jquery_update structures the versions it ships with IIRC).

@ghost
Copy link

ghost commented Oct 10, 2014

We can start by giving the /libraries folder priority over /core/libraries for easy replacement and then add an admin page to allow site builders to select from all the available versions to use from the UI.

Ooh, I like it!

@jenlampton
Copy link
Member

I really like where this discussion is going, but since we need to get 1.0.0 out soon, how about we split this issue into two?

  1. decide on a standard place (naming convention) for where we keep libraries
    I propose we go with @klonos's suggestion of using /core/libraries and /libraries/. If we go with that convention, we could also adapt backdrop_get_path() to work for libraries as well as modules, themes, and layouts.

  2. add an admin UI for control over which libraries (& versions of) are in use
    I'd like to see this module develop and mature in contrib before we move it into core. We could either start by porting libraries module to Backdrop, or start fresh with a simple admin UI module.
    See Add an admin UI for control over which libraries (& versions of) are in use #462

@jenlampton jenlampton added this to the 1.0.0 milestone Dec 16, 2014
@jenlampton jenlampton changed the title Standardize on a place for shared libraries to be used by multiple modules. [DX] Standardize on a place for shared libraries to be used by multiple modules. Dec 19, 2014
@docwilmot
Copy link
Contributor

Noted that this says need change notice. Was that an error? I don't see the API change.

@quicksketch
Copy link
Member

Yeah looks like a mistake. We haven't even implemented anything yet, so I don't see how it could need a change notice. I removed the tag.

@jenlampton
Copy link
Member

whoops, sorry about that :)

@quicksketch quicksketch modified the milestones: 1.x-future, 1.0.0 Jan 7, 2015
@quicksketch
Copy link
Member

I don't think we'll implement this in the next week (and I'm not sure we should even if we could). However, there's nothing keeping this from being in a minor release. So moving to the 1.x-future milestone until we get a timeline for this.

@jenlampton
Copy link
Member

I think the first three remaining tasks (see main issue) we could definitely do before the 1.0.0 release if someone wanted to do it. It's a minor task to move the files and add some documentation. The 4th part we should postpone, and could be moved to a separate issue if 1 - 3 get done here. If not, 1.x-future it is :)

@oadaeh
Copy link

oadaeh commented Feb 5, 2015

I'm going to hold off on the Libraries API port (https://www.drupal.org/node/2417985) until this issue gets sorted out, as that may save me a bit of work. :) I have other things to keep me busy in the mean time.

@jenlampton
Copy link
Member

@oadaeh We'd love to see a port of Libraries API for Backdrop. As far as what's left of this issue, it's just a little core cleanup and some documentation, so nothing that should block the port of that module, at least not for the 1.x cycle of Backdrop.

@klonos
Copy link
Member Author

klonos commented Feb 6, 2015

We could probably add something like Libraries API to core in the 1.x cycle of Backdrop.

Yes please! I can't remember any of my D7 sites not using it.

@klonos
Copy link
Member Author

klonos commented Mar 29, 2015

Another thing is to move jQuery & jQuery UI (and other 3rd party libraries perhaps?) under /core/libraries.

@docwilmot
Copy link
Contributor

welcome back @klonos, the place has been way too quiet recently.

@klonos
Copy link
Member Author

klonos commented Mar 29, 2015

Thanx @docwilmot, I've been working crazy hours and still have another 3 weeks to go. So, I won't be around much. Especially during weekdays. Just managed to find some spare time throughout the weekend. So much to catch up and unfortunately us humans need sleep 😄

@docwilmot
Copy link
Contributor

Its been a while and after @jenlampton's post about contrib libraries recently, I finally recalled this conversation here.

@quicksketch @jenlampton a few queries please:

  • how do we reconcile the presence of hook_library_info and and entry in system table for libraries? Would system invoke the hook prior to saving in the system table?
  • system saves the .module or .info file as the $filename. What would we save for libraries? hook_library_info declares js and/or css files; which one? Or do we save the library name, and serialized the rest of the info?

I note also the conversation at #1123, which may influence this one. Would be nice to get this PR'd already. It been a while.

@quicksketch
Copy link
Member

how do we reconcile the presence of hook_library_info and and entry in system table for libraries? Would system invoke the hook prior to saving in the system table?

I don't think the hook would be invoked prior to creating the entry in the system table, because modules may actually use backdrop_get_path('library', 'foo') in their hook_library_info() callbacks. You'd end up with a loop in such situations if the hook were called to populate the system table.

system saves the .module or .info file as the $filename. What would we save for libraries? hook_library_info declares js and/or css files; which one? Or do we save the library name, and serialized the rest of the info?

Maybe the bower.json, package.json, or other info file for the library (if found)? Otherwise maybe we just make up a placeholder like [library_name].info, even if doesn't exist in the directory.

@docwilmot
Copy link
Contributor

Modules and themes and layouts have a standard naming structure: we search for that, find them, put them in the database. For libraries we need a way to search through the expected places, build a list of libraries there and put them in the database so that backdrop_get_path() works. Would the directory name be enough for this? Because there is no standard indentifying file like an .info file. That would also ruin the idea of supporting multiple versions at once (with a UI, someday).

Maybe the bower.json, package.json, or other info file for the library (if found)?

By "if found" you mean declared in the library_info file? I have 5 libraries on my install right now, all of which are a bit different, and none have bower etc.

@Graham-72
Copy link

I don't know whether this is at all relevant but the Libraries API module ported from Drupal (backdrop-contrib/libraries) contains a great deal of code to deal with different library locations, variants and versions, and I confess I understand very little of this.

In Drupal it provided for multisite situations by using conf_path() but I found this to be a problem in Backdrop because with function find_conf_path we have " If not using multisite, the path returned will be a single period (indicating the current directory)." It also provides for looking for libraries in a profiles directory - will we have this in Backdrop?

The results of Libraries API search for libraries would seem to be stored in a datatable 'cache_libraries'.

@Gormartsen
Copy link
Member

Hi guys! I would love to push it up.

@Graham-72 I se that one year ago you ported libraries module but there is still some undecided situation where we are going with it, right?

I had a question why we don't use composer for libraries dependencies and etc.

Since we have project installer that install module from admin part, we do need to make sure that all libraries for project get installed as well.

Anyway, could anybody give me some status links to understand if I could be helpful here.

@biolithic
Copy link

Hi Gor,
I thought I remember a founding forker saying previously that we wanted:

  1. to maintain compatibility with D7
  2. make it easier for people who are not so command-line savvy
    ("non-developers")
  3. possible license issues which I'm not sure I understand

so we used the libraries D7 port from Graham with the manual dependency
uploading for our solution. This is what I remember from the status and
decision.

On Tue, Jun 21, 2016 at 9:03 AM, Gor Martsen [email protected]
wrote:

Hi guys! I would love to push it up.

@Graham-72 https://github.com/Graham-72 I se that one year ago you
ported libraries module but there is still some undecided situation where
we are going with it, right?

I had a question why we don't use composer for libraries dependencies and
etc.

Since we have project installer that install module from admin part, we do
need to make sure that all libraries for project get installed as well.

Anyway, could anybody give me some status links to understand if I could
be helpful here.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#159 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABJ8DtHBmkOc49F2zXw5AEsv_j762AKWks5qN-8ugaJpZM4BY0If
.

@Gormartsen
Copy link
Member

@biolithic thanks!

@laryn noticed issue with https://backdropcms.org/project/libraries

It is access denied page.

@jenlampton
Copy link
Member

jenlampton commented Jun 21, 2016

@Gor @laryn I believe that node was unpublished since there was a time when we didn't want laypeople using the libraries module for Backdrop. I just re-read this issue and it looks like that is not the case anymore (at least temporarily, until we get this functionality into core, woot!) so I have re-published it.

I also copied the current non-API-breaking plan from @quicksketch's comment into the main issue so everyone can see the tasks ahead of us.

@Gormartsen
Copy link
Member

Tnx @jenlampton

@docwilmot
Copy link
Contributor

docwilmot commented Jun 21, 2016

@jenlampton I notice Make a decision where libraries should live is checked. Did we decide? Are you referring to this from @quicksketch

core/libraries
libraries
sites/[site_name]/libraries

I ask because I would think that the last point you made

Do something fancy with backdrop_get_path()

would be a prerequisite for doing the Move core libraries to this location, no? Can we move all core libraries to one location without fixing backdrop_get_path() to be able to find them?

@jenlampton
Copy link
Member

jenlampton commented Jun 21, 2016

Can we move all core libraries to one location without fixing backdrop_get_path() to be able to find them?

Well technically we can, but I don't think we should.

Those old todos were fairly vague and not all that helpful, so replaced them with the more detailed non-API-breaking plan from @quicksketch's comment.

@Gormartsen
Copy link
Member

@quicksketch I was thinking about implementing type=library .

It does make sence due libraries are separated entities and modules can be depend on it.

From another side, it is going to be bottle neck. we don't have human power to implement so much new extensions like type library and keep them up to date.

There is a lazy path I think. Composer is very popular right now and many libraries get supported by it. Also it handle versions as well.

So how about to have one module, that will respond on hook_requirements check and if module require library NAME, version VERSION - it will check composer and if it is available - download and register as library.

@jenlampton
Copy link
Member

@Gormartsen see @quicksketch's note about library_read_composer_version. I put the relevant info in the first post for easier access.

@docwilmot
Copy link
Contributor

Well technically we can, but I don't think we should.

Should is what I meant yes.

So how about to have one module, that will respond on hook_requirements check and if module require library NAME, version VERSION - it will check composer and if it is available - download and register as library.

That sounds like it would be magic. Would simplify things for the nontechnical persons significantly, and also make Installer more useful in those cases.

@jenlampton
Copy link
Member

jenlampton commented Jun 21, 2016

That sounds like it would be magic. Would simplify things for the nontechnical persons significantly...

Magic yes, because this step requires that wherever it is that people are working would be able to use composer in the first place. If not, that would be a HUGE increase in complexity for nontechnical persons... Not something we want to burden our target user with, I don't think. But a developer tool for building new modules? sure! Maybe even work it into devel?

Anyway, we're getting a bit off-topic and into contrib-land here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants