backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

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

Open klonos opened 10 years ago

klonos commented 10 years ago

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:

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

quicksketch commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

...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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 9 years ago

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 https://github.com/backdrop/backdrop-issues/issues/462

docwilmot commented 9 years ago

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

quicksketch commented 9 years ago

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 commented 9 years ago

whoops, sorry about that :)

quicksketch commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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.

jenlampton commented 9 years ago

@klonos me too, but that's because it's required by a bunch of contrib modules... not sure we'll have the same problem on GitHub since we are allowed to bundle libraries in ways we weren't on drupal.org

klonos commented 9 years ago

Bundling the libraries was not the actual problem in Drupal. The contrary, one of the main issues was the need to be able to decouple specific versions of libraries from the modules they shipped with and the ability to easily replace them with newer versions as they came out because module maintainers often could not keep up with the pace. (wow, big sentence)

Another major headache was the fact that without it module could not share the same library.

jenlampton commented 9 years ago

Modules often depend on specific versions of libraries. If you replace the Flexnav 1 jQuery library with Flexnav 2, the flexnav module will break - it ships with the version it supports for a reason. The module code may need to be updated to work with the version of the library you want to use, the library version is not something that should be left to end-users to decide.

Additionally - a site using two modules that each bundle the same library is an edgecase. If you are using two modules that bundle the ckeditor library (for example) then you are most likely doing it wrong. :)

klonos commented 9 years ago

Yeah, but on the other hand libraries might be updated faster than what the modules that use them do. Some of these updates are serious security/bug fixes. The point is to decouple the libraries from the module releases because in Drupaland people had to keep manually updating the shipped libraries till they got merged in the official release of modules.

As for the breakage that might occur by updating to a newer version of a library, this is a possibility, yes, but if that happens, people can easily revert to the previous library. Having an easy way to test-upgrade and revert if need be without touching the actual module would also help people report "heads-up" issues in module queues about incompatibilities/changes in upcoming versions of libraries.

True about the two modules using the same library, but selecting CKEditor is a convenient example for making your point. jquery.hoverIntent and jquery.bgiframe on the other hand are examples of libraries that could be used by many (menu-related) modules or themes and I honestly don't see a reason for each module or theme to bundle a different version of the library.

docwilmot commented 9 years ago

We need to decide on this. Many D7 modules depend on Libraries API; if current ports are using alternative methods, it means it will require a mass refactoring of a significant chunk of our contrib if/when this gets done. Thats unacceptable. We should at least decide now

edit: by 'this' I dont mean the standardized place for libraries, but the associated API.

quicksketch commented 9 years ago

I propose the following portions of implementation.

So in short, we'd provide the most central functionality of Libraries API:

This approach also wouldn't change any core APIs; it would be purely additive. And module developers are left to their own liberties if they want to bundle a library or not, or optionally bundle but allow overriding.

klonos commented 9 years ago

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

docwilmot commented 9 years ago

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

klonos commented 9 years ago

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 :smile:

docwilmot commented 9 years ago

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:

I note also the conversation at https://github.com/backdrop/backdrop-issues/issues/1123, which may influence this one. Would be nice to get this PR'd already. It been a while.

quicksketch commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 8 years ago

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 commented 8 years ago

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 notifications@github.com 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 https://github.com/backdrop/backdrop-issues/issues/159#issuecomment-227449543, or mute the thread https://github.com/notifications/unsubscribe/ABJ8DtHBmkOc49F2zXw5AEsv_j762AKWks5qN-8ugaJpZM4BY0If .

Gormartsen commented 8 years ago

@biolithic thanks!

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

It is access denied page.

jenlampton commented 8 years ago

@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 commented 8 years ago

Tnx @jenlampton

docwilmot commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

docwilmot commented 8 years ago

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 commented 8 years ago

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 :)