Hi,

I'm not sure if this is best practice or what, but when files are removed from a module package, the module is probably updated by removing the originally installed directory and then extracting the tar of the new release. When that happens, one may forget to save/move the external editor package, that is required to be present by this module in the same module location.

So... it might be easier if the module could provide alternative locations to install external editor packages. Maybe somewhere in the files directory? Maybe in the modules directory with no real Drupal module in it? Maybe somewhere in the themes directory?

I wasn't sure if this is more a support request than a feature request. Suggestions to make the upgrade path easier?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Title: Separate location for external WYSIWYG editors? » Separate location for editor libraries

Better title.

markus_petrux’s picture

What about checking for editor packages using the same method Drupal modules/themes are checked?

Here's a function that could do this:

/**
 * Obtain the directory for an editor package.
 *
 * For example, packages can be located in:
 * - sites/all/wysiwyg/tinymce
 * - sites/example.com/wysiwyg/fckeditor
 *
 * @param string
 *   Name of the editor package. ie. 'tinymce', 'fckeditor', etc.
 * @param string
 *   Additional file/directory name that should exist within the
 *   editor directory. ie. 'jscripts/tiny_mce'
 */
function wysiwyg_get_path($name, $file = '') {
  static $editors = array();

  if (!isset($editors[$name])) {
    $location = conf_path() ."/wysiwyg/$name";
    if (!empty($file)) {
      $location .= '/'. $file;
    }
    foreach (array($location, preg_replace('#^(sites)/.*?/(.*)$#', '\1/all/\2', $location)) as $file) {
      if (file_exists($file)) {
        $editors[$name] = $file;
        return $editors[$name];
      }
    }
    $editors[$name] = FALSE;
  }

  return $editors[$name];
}

Then, in wysiwyg/editors/tinymce.inc it would be used like this:

    'library path' => wysiwyg_get_path('tinymce', 'jscripts/tiny_mce'),
sun’s picture

Hm. The idea sounds not too bad. So admins could provide different editors for different sites on a multi-site install.... I'd like to get some opinions from others.

markus_petrux’s picture

In the meantime, I managed to implement a clone of the builtin tinymce editor on a separete module. Here I'm loading tinymce.inc, calling wysiwyg_tinymce_editor() and changing editor settings to use a different location of the TinyMCE package, settings things up for the needs the site I'm working on right now. I had to clone tinymce-3.js as well, because the attachment method for my cloned editor was not invoked from wysiwyg.js (reason is the editor name is based on the module name, which is different in my case). It's easier now (for me at least) to upgrade 6.x-1.x-dev.

Cheers

sime’s picture

Component: Wysiwyg Editor » Plugins

I consider this issue from a wider perspective. It dawned on me when I maintained SWF Tools (with swfobject, flash players, mp3 players) that it was just messy putting plugins into a sub-directory of swftools.

So we're just storing a path yeah? Where we store the plug-ins is just a convention in some ways.

A new convention
So the convention you set with WYSIWYG API is simply telling your users to place 3rd part code in directories called "plugins" in sites/all (or optionally sites/default or sites/x.y.z).

(Markus, I wouldn't choose the files directory because some plugins components and need executing (pdf writers for eg) and in the files directory these can't be executed by design. Plus imagine .htaccess files spread throughout fckeditor!)

I like the auto-discovery of WYSIWYG API, but just to be clear, as a convention, the above is enough. It wouldn't be too hard to encourage other module writers to take up the practice.

Helper function
Then, the sweeteners, you can make it easier with a scanning function like Markus' above. I'd suggest call it drupal_get_plugins(). And be able to pass "fckeditor" and match:

"fckeditor"
"fckeditor_hacked"
"fckeditor.2.6.4"

It should also be possible for the tools to be organized in sub-directories.

sites/all/dompdf
sites/all/plugins/flash/1pixelout
sites/all/plugins/flash/wijering/image_rotator
sites/all/plugins/flash/wijering/media_player
sites/all/plugins/wysiwyg/fckeditor
sites/all/plugins/wysiwyg/tinymce
sites/all/plugins/javascript/... 

drupal_get_plugins() would sometimes match multiple tools (fckeditor.1.2.3, fckeditor.4.5.6), so it would be up to the module writer, and their UI/Instructions as to how to handle that. I'd suggest that drupal_get_plugins() can be passed parameter that specifies the path should be matched exactly per the INSTALL.txt instructions so that it will never match more than one plugin.

Then I'd suggest setting a variable for each plugin to allow site builders to use the $conf array in settings.php to force the path if desired.

Wrap up
With just one function, and some altered install instructions, you've made it possible to

  • reduce issues when people nuke their plugins on upgrade
  • raised the possibility that every module that needs swfobject (etc) auto-discovers the same one
  • made a Drupal 7 improvement that is a no-brainer
  • helped Drupal shops build a favourites collection of 3rd party plugins, organized the way they want.

I think Acquia would like this too, as a convention, as their business model does require minimizing weird issues like users nuking their plugins (see. /modules/acquia/print/lib/ in acquia distro.)

And that's all folks!

Darren Oh’s picture

Component: Plugins » Miscellaneous

Maybe we could have "modules", "themes", and "libraries" as standard directories.

Darren Oh’s picture

Component: Miscellaneous » Plugins
aaron’s picture

sun’s picture

I specifically did not point to that core issue, because the way "plugins" are discussed there is something completely different. Modules don't ship with external libraries and external libraries do not ship with .info files. Period. The entire proposed design for "plugins" does not apply.

Thanks for the ideas and feedback so far. I need to think a bit about this.

@sime: Upfront - Wysiwyg intentionally does not support multiple versions of the same library and directs users to put "the library" (whatever version it is) into a certain directory (internal library name). If that exists, Wysiwyg API determines which version it is and based on that, what features are available and how to interact with it. This boils down to: Drupal (here: Wysiwyg) plays a passive part, the functionality depends on the actual library that has been installed. Is this something that could work out for SWF Tools as well? How do you account for API/feature changes in external library versions there?

Chris Johnson’s picture

Modules don't ship with external libraries and external libraries do not ship with .info files. Period. The entire proposed design for "plugins" does not apply.

sun, I don't quite understand what the above means. Could you clarify for me? Are you simply saying the core jQ proposed solutions don't apply to the plugins discussion here?

I maintain a couple modules which require 3rd party libraries. Unlike editor plugins, perhaps, they are not optional or swappable; they're required. Yet they also need a standard place to live, similar to SWF and editor plugins. Is there some reason we shouldn't make a more general solution which will fit both cases?

Thanks.

sime’s picture

@sun SWF Tools works pretty much the same.

Just summarizing my thoughts if I may:

1. Deep directory scanning is important for "niceness" - you allow site builders to be pedantic about how they organize their tools.

2. Crucially we need the ability to set paths - this will enable work-arounds for edge-cases:
$conf['plugins']['fckeditor'] = 'somewhere/else/fckeditor';
$conf['plugins']['mediaplayer'] = 'http://mymediaserver.com/flashplayer.swf';

3. And in hindsight, accommodating different versions of the same tool is not important, it's an edge-case that could be solved with the 2.

sime’s picture

Re #315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries). There is zero mention of things like "flash", "swf", "pdf(writer)". Which is ok.

So that said, I withdraw everything I've said in this thread about core and jquery plugins. It's moot. We really just need to define a loose convention and a simple discovery technique.

sun’s picture

@Chris Johnson: The core "plugins" issue/approach presumes that required and/or optional external libraries are shipped with the module itself. Thereby, modules are able to define 'version' => '0.95-beta' upfront. Drupal plays the active part there, and Drupal supports a certain, expected version only. That is only possible if the external library is shipped with the module. If the library is not contained/pre-packaged with a module, Drupal can only play the passive part. Otherwise, if the user fails to install the proper library version, the application won't work as expected. If the user wants to use a different version, Drupal won't support it.

I don't think there is any difference in required and optional libraries. For me, it boils down to version detection and dependency handling. You can't enable or use features of a (required or not) library if the library is not there. Equally, if you can't detect its version. Equally, if the detected version cannot be handled or is not (yet) supported. And, depending on the version that was detected, certain features might be available or not. If you start to integrate features of an optional library into your site, the library becomes de facto required. So, "required" has a completely different meaning here.

@sime: Your (and everyone else's) thoughts are more than welcome.

1. Deep directory scanning: While being "nice", this adds a fair amount of complexity to the library detection process. Any file directory scanner function (like drupal_scan_libraries()) can't rely on .info files or any other 'unique' things. This means that 'something' needs to be found 'anywhere'.

We could argue: Modules implementing external libraries should know the contained filenames at least. However, returning the proper directory path for the library based on a file that was found somewhere might not work in all cases; think of a library that ships with foo/foo.bar.js and foo/packed/foo.bar.js. Just a silly example. (And external libraries tend to be silly.)

We could argue: Modules implementing external libraries should know their folder name at least. However, there are libraries (like FCKeditor, to provide a real world example) that use their own library name as a sub-folder in the library itself (fckeditor/fckeditor/...), whereas the sub-folder is not the root directory of the library, only used internally.

That's why I went with a simple, non-recursive directory structure - forcing users to create/name the library root folder properly (as explained in the on-site installation instructions).

2. Custom path overrides are a good idea and should indeed be possible, yes.

Re #12:

Yes, definitely. However, my (personal) goal is to make the entire external library handling easier for all contrib modules. So how about a multi-step roadmap?

i) Standardize on a location. Voting for sites/all/libraries to avoid conflicts with any possible jQuery plugin manager in D7 core, but also call it precisely (Drupal modules are "plugins" as well). No multi-site support, just test whether sites/all/libraries/[my_thingy] exists; leaving further compatibility handling and dependency checking to the implementing module. I.e., all as-is, just a different (safe) library base path.

ii) [Mainly my main interest] Develop some functions that allow to scan sites/all/libraries for available libraries. Investigating potential solutions for recursive scans. Potentially abstracting version detection and (rather upcoming) dependency handling of Wysiwyg API into a generic API/toolkit. Very potentially ending up in a separate module to allow countless of others easy inclusion.

iii) Let it mature. Enhance for jQuery libraries. Move into core.

Magnity’s picture

+1 for sun's roadmap, especially [i] and [ii] - even the informal step of agreeing to use the sites/all/libraries path, I believe will make things so much easier.

If others agree with that, it'd be good to see a few module maintainers commit to using this.

sime’s picture

Awesome, simple, doable. I'll help with advocacy.

sun’s picture

Version: 6.x-1.x-dev » 6.x-2.x-dev

Damn. You know. I secretly thought of using http://drupal.org/project/library as namespace. But well. Next idea: I'll talk to merlinofchaos whether he'd accept a library.module in the CTools package, on which Wysiwyg API will soon depend anyway, and which is required on most sites for Views/Panels/Popups/etc. already.

Any other suggestions for this namespace/project issue?

Aside from that: Great! So let's go for it. I'll post a patch for Wysiwyg here shortly, if no one beats me to it.

markus_petrux’s picture

Just a thought, in regards to "No multi-site support" in #13, point i).

I would say, why? For example, let's say I have a multi-site installation with my production site and a development/test site. Is it possible that I may need the same library with different versions? For example, let's say my production site is running foobar v1.x, and I need to test foobar v2.x on that installation along different versions of contrib/custom modules that depend on a particular version of foobar.

What's the problem of providing multi-site support? Maybe the module implementing this stuff could easily use the same pattern as Drupal when looking up for modules/themes, and it would need some kind of registry so that scans are not performed every now and then.

sun’s picture

Well, sure, for multi-site support, it would need to work like http://api.drupal.org/api/function/drupal_system_listing/6. Depends on the module though, whether multi-site support makes sense at all. I could imagine, for Dynamic Rendering, PHPMailer, SWF Tools players and stuff, it might not be useful at all, because the module currently supports a certain version of the external library only or requires you to put different versions of the library in folder names with a version suffix.

For Wysiwyg API, jQuery UI, and similar modules, it makes more sense.

I will certainly investigate that, but in the end, the module's code might need major tweaks to allow for completely different locations, and feature-wise that belongs to a more generic API/toolkit already.

soxofaan’s picture

(subscribing)

I'm maintainer of the GeSHi filter module which requires the third party PHP library GeSHi (Generic Syntax Highlighter). This module currently looks for the library in a subdirectory of the module's directory (drupal_get_path('module', 'geshifilter') .'/geshi') but lets the admin override this (with a simple text field on the GeSHi filter settings admin page).
I'm interested if there would be a conventions/standard to follow.

Concerning the naming of the folder:
I don't like sites/all/plugins: what's the difference between "module" and "plugin" and how do you explain this to a non-technical user?
sites/all/libraries is better, but I think it doesn't communicate the "thirdpartyness" enough.

Why not just go for the explicit sites/all/thirdparty?

markus_petrux’s picture

hmm... let's say we call this thing "components". The module that implements this stuff, could build the registry based on hook_components_info() where modules can return the name of the components they are interested in.

For example, Wysiwyg API editors could do something like this:

// @return array of component names required by the module implemeting the hook.
function tinymce_components_info() {
  return array(
    'tinymce',
  );
}

That could be used to scan the sites folder for sites/site-a/components/tinymce, sites/all/components/tinymce, ... and save the results of all components in cache table. That would be the components registry.

Then, the tinymce editor could ask for the path to the tinymce component with something like:

$tinymce_path = components_manager_get_component_path('tinymce');

Et voilà! :)

Edit: The location of the components could include the module name. for example: sites/site-a/components/wysiwyg/tinymce, sites/all/components/wysiwyg/tinymce, ... that would open the option to have installed several modules that make use of different versions of one particular component. And it would be possible to symlink them if using the same component would be acceptable, compatible.

Magnity’s picture

My definite preference would be for libraries both for the folder sites/all/libraries and for the name of any module that helps autoseek them.

However, the majority of my usage would be part [i] of sun's roadmap in #13, and this would be more dependant on a number of maintainers agreeing rather than it being a particular name - therefore i'd be happy as long as long as we could settle on a pseudo-'standardised' solution!

sun’s picture

I thought (but maybe I'm mistaken), we agreed on sites/all/libraries already?

@soxofaan: What's third-party, what not? All contrib modules are third-party. Files that have been added by your users, ending up in sites/all/files, are third-party data as well. Third-party means quite a lot.

soxofaan’s picture

I see third party as "stuff" that didn't come from drupal.org repositories/project pages. Typically code that isn't/can't be (re)distributed through drupal.org because of licencing/redistribution issues. So I don't consider contrib as third party, but I can understand that other perspectives on this can differ.

I have no real problem with sites/all/libraries, I will settle with what will be decided (I didn't know it was already agreed on).
"Libraries" just sounds rather close to core to me. And "plugins" too close to "modules". I just wanted to chime in with a suggestion, I didn't mean to bikeshed this.

sun’s picture

Note that in Wysiwyg API's case, some editors are distributed under the GPL, or using a license that would allow conversion and redistribution under the GPL. However, licensing issues are not the reason why those libraries are not hosted/redistributed via drupal.org. The primary reason is: These are very complex development projects, having their own home, their own bugtracker, and their own support forums. Duplicating all of that would not make sense in any way. Instead, it would lead to bloated issue queues, because users would not understand that Drupal integrates those libraries only, and features/bug reports need to be submitted to the library's project instead.

That said - as long as the first module did not implement sites/all/libraries as new location, I'd say we can certainly bikeshed this. I'm open to further suggestions, but also very happy with libraries already.

From a technical perspective, the term is very concise, because libraries, contrary to modules, do nothing on their own. We could make it a bit shorter and geeky by naming it just libs, mirroring *nix-based operation systems. Not sure though, because we usually do not abbreviate terms in Drupal. However, that term would also be available as project namespace.. ;)

markus_petrux’s picture

+1 for libraries.

go, go, sun go for it! ...also add it to the list of things that need to be in core. :)

sun’s picture

Just realized another nicety - this also allows you to setup sites/all/libraries as vendor branch in your versioning system. :)

sun’s picture

Status: Active » Needs review
FileSize
15.84 KB

Here we go.

Wysiwyg API is lucky, because it already implements wysiwyg_get_path() all over the place. So this was a very simple target to implement this stuff.

sun’s picture

I wonder whether and how we could inform users upgrading the module about this patch change. Ideas, suggestions?

Or is it sufficient that, after updating, the profile overview page will display "Not installed." along with the new directory as location for all editors?

sun’s picture

Status: Needs review » Fixed

Sorry. Sometimes I'm taking development too seriously.

Thanks for reporting, reviewing, and testing! Committed to all 2.x branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

sime’s picture

Woohoo! (OK I'm a cheap date, small victories.)

Later this week, I should get a howto for other modules to use a "libraries" folder.

markus_petrux’s picture

Wouldn't it be better implemented as an external module so that other modules can make use of it?

As per the method to tell users about the change... maybe using hook_requirements('runtime') ?

sime’s picture

@markus_petrux I think it's important first to establish the convention. You could certainly create a module to do this (perhaps we should) or maybe modules could just choose how they want to do it, as sun has. As long as we all use the "sites/*/libraries" convention.

For example other modules might go minimal and just use $path = variable_get('plugin_path', 'sites/all/libraries/foo-module/plugin.swf');, with corresponding INSTALL.txt instructions.

markus_petrux’s picture

Sure, but if this was implemented via a separate module, then it could provide ready made APIs to access the libaries folder, it could provide a central place for user side, and developer side documentation, it could be easily introduced in other modules that require 3rd party libraries, it could provide an abstraction layer to exempt modules from having to reinvent the wheel, it could be easily introduced in core, if this proves to be a valid pattern, etc. :)

sime’s picture

Agreed. But sun is a busy boy! Let's say if you start a project I'll happily help you out in the issue queue. :P

markus_petrux’s picture

I thought sun was going to start something in that direction...

Well, if this is prone to be included in core some day, maybe we should think "core". So maybe, if it was in core, then it would be something like drupal_get_path('library', 'tinymce'). But this would require a file in the library directory with the name tinymce.library, which is not desired here, so maybe core would have to provide a separate API such as drupal_get_library_path('tinymce'). If so, then it would be easier to implement as a transitional module.

One possible way to approach this, since D7 is not frozen yet, maybe we could post a feature request for D7, and backport that to D6 in the form of a formal patch, or maybe as a trasitional module. But this would be a slow route... so maybe simply implementing a separate module now for D6 that provides the API, something like library_api_get_path('tinymce'). This is all contrib modules need, then the inners can be extended, optimized at any time, and finally, it could be included in core one day. If not D7, maybe D8...

Once this is done, it will probably need volunteers for documentation and then spread the word, so other module authors know this tool exist and can make use of it.

Maybe the fastest method now would be going for a separate module, maybe library_api, library_manager or similar.

@sun: Would you agree on using a separate module for this? Would you like to create such a thing?

sime’s picture

merlinofchaos is also open to implement this in Chaos Tools. I'd be happy to see any and all approaches, but the risk in shooting for core immediately is that many will identify overlap with #315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries).

So, if the library convention is popular first, then it becomes a candidate for an easy partial implementation of #315100 in case that patch is failing to hit D7, and also allows us to step into #315100 with a clear use-case that isn't just about jquery plugins.

Ok, I'm probably overly-thinking this :)

sun’s picture

Title: Separate location for editor libraries » Libraries API
Status: Fixed » Active

Wow. I go to sleep and you go nuts. :P

I...

  • prepared those functions already for easy extraction. Other modules can easily copy + rename (fork) those as long as there is no generic helper module for library handling.
  • am tinkering intensively about the functionality, and, to which extent we can or want to extract library handling from Wysiwyg API. The point being: We are already using a fork of Panels' plugin handling for editors and Drupal plugins (to be rewritten to use CTools instead), which now has external library handling in the mix. Furthermore, I'm thinking about recursive dependencies of external libraries, meaning that native plugins shipped with an external library could be handled identically to the library itself (see #328252: Handle compatibility of internal plugins via plugin API).
  • am still unsure which namespace to register, flip-flopping between 'lib', 'libs', and 'libraries', being in favor of the latter. When this project/module exists, Wysiwyg API will add a dependency on it. Some feedback about this point will allow us to register the namespace and create an architectural design task issue for discussion (perhaps as well move this one).
  • also re-considered that the CTools project would probably not be a good home for this module, because of different release cycles. (We are in flux while CTools matures)
  • still do not want to think about Drupal core. I left many arguments in #315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries) for not doing so, and still, this functionality does not need to be in core to be used and prove itself in contrib. We introduced a different location and thereby a paradigm change now. At least in my mind, all of this is very much in flux yet. We need to test whether it works for arbitrary libraries (not JS, but e.g. PHP) and come up with a sane API.

Well. Now that half it is written, let me add the rest. ;)

  • What definitely needs to be in Libraries module is wysiwyg_get_path() and wysiwyg_get_libraries(). The latter I originally called drupal_libraries_listing(), because it's a straight fork of drupal_system_listing() (on that note: Drupal core provides no function to scan for directories [not files]).
  • I think, something that really helps users are Wysiwyg API's on-site installation instructions. That, I imagine, would be good to provide as API function, so all modules can use it for their libraries.
  • However, the libraries listing only scans for directories. Even the on-site installation instructions require some basic definition for each library, and, a callback to determine the library version. So, I think this is the extent the Libraries module would have to take over.

Thoughts?

sime’s picture

agreement - need to make some space to have a play.

sun’s picture

The directory is called 'libraries', we are talking about handling 'libraries', it's an API for libraries, so http://drupal.org/project/libraries is plain logical to me. Registered.

markus_petrux’s picture

It would be nice if other module author in need of such a feature were involved in this discussion. This will probably help to draw a minimum set of APIs that are needed. What's not common to all, then it could be implemented by each module separately, but the parts that are common could be moved to a project that could be created now. These common parts could be just these library_get_path() and library_listing(), or something on that sort. Then add more APIs as the need arises.

First thing is decide a namespace. Once this is done, create a project, and then that could be the place where all the rest can be discussed.

Another approach: move this discussion to g.d.o. and spread the word, so that module developers in this kind of repository can get involved and, decide namespace, create a project, implement a minimum set of APIs, and so on.

Honestly, I think all this is pretty simple. What we really need to normalize is a repository for all these libraries. The rest of the stuff related to them, versioning, features, compatibility issues, etc. could be resolved by each module. For example, if Wysiwyg API needs a separate library for TinyMCE 2 and 3, then just do library_get_path('tinymce-2') and library_get_path('tinymce-3'), respectively. What the library API helps you to abstract is the phisical location of these libraries in a way that's easy to manage for site administration and transparent to modules that use it.

I wouldn't mix any other feature that complicates things here, at least not initially, until the API matures based on real needs of module developers and users, but we won't be able to completely see the whole picture until we're one step further than nothing, which is where we are now.

Well, that's just my 2 cents now. :-/

Edited: This message has been written before reading the previous one by sun, which is a great thing! :-) Now we have a place were to discuss further stuff, and involve more module authors.

sun’s picture

a) I'm not sure whether mail subscriptions to this post will still work if we move this issue to the new project. I'd like to invite everyone here to check "all" on http://drupal.org/project/issues/subscribe-mail/libraries, so we may split this discussion easily into separate issues.

b) As nice as library_get_path() is, we can't use it, due to http://drupal.org/project/library. However, I would even consider to prefix API functions with 'drupal_'. Not sure yet.

c) We have a fair amount of developers/projects dealing with external libraries on this issue already. Just counting my projects: Wysiwyg, PHPMailer, Dynamic Rendering, jQuery Update, jQuery UI (which I want to tackle in this order). All of them could use on-site installation instructions like Wysiwyg provides. Adding GeSHi syntax highlighter (soxofaan), SWF Tools (sime), Flash Gallery (sime), Sparkline (Chris Johnson) to the list. More would indeed be helpful.

d) Related question: Do we want to make Libraries API depend on CTools, or should it work without?

soxofaan’s picture

I'd like to add the following devil's advocate opinion to the discussion:

Refactoring stuff to new contrib modules is nice for code reuse and whatnot, but adding dependencies to modules have their disadvantages too:

  • It adds burden for the site builder/admin: (s)he has to do extra research (is this module stable enough for my site?), downloading, installing (second degree dependencies?), tracking of updates (will an update break anything?), etc
  • apart from the obvious code reuse and API abstraction advantages for the module developer, it also makes some things a bit harder, like support, testing and debugging.

Don't understand me wrong: there are a lot of examples where the advantages outweigh the disadvantages.
But in this case, as a module developer, I'm mainly interested in just one function call: give me the path to this 'foo' library. Depending on another contrib module for this seems like a bit overkill to me.
I haven't looked deep into it, but I would much rather use the suggestion of sun in #37:

Other modules can easily copy + rename (fork) those as long as there is no generic helper module for library handling.

Of course, if this function would be in Drupal core, I wouldn't hesitate a second to (re)use it.

To conclude: I welcome and support the standardization of the location, I would prefer to see the related functions in Drupal core (7 if possible of course) and I am hesitant to depend for this on an extra contrib module for Drupal 6 (given the advantages/disadvantages balance).

</towcents class="devilsadvocate">

sun’s picture

@soxofaan: I can see your point. However, that point is mistaking the cause and the effect. Only if a certain amount of contrib modules makes usage of this API module, we can be sure that its design works out in the real world - and that is the primary indicator for moving something into Drupal core. I hate examples, because they oversimplify things and usually don't cover all aspects, but it is pretty similar to Token module -- now that countless of modules require it or implement optional support for it, only now we know that its current design hurts performance and is not flexible enough for certain scenarios (read the Tokens in core issue for more info). Regarding external libraries it becomes even more special: Drupal core itself does not need this feature, so we would blindly add something to core that didn't undergo extensive testing.

soxofaan’s picture

@sun: I know that the gates to Drupal core are fiercely guarded (and I consider this a good thing).
But I (from the standpoint of module developer that would use it) would not consider a function like libraries_get_library_path_whatever() to be the same as a module like Tokens. The latter is around 900 lines of code and integrates on different levels with Drupal and contrib, while I expect the former libraries_get_library_path_whatever() to be somewhere between 20 and 40 lines of code. Of course you can add many bells and whistles, but I don't think there is a defined need for this. So, yes, I hate your example too ;-)

Only if a certain amount of contrib modules makes usage of this API module, we can be sure that its design works out in the real world - and that is the primary indicator for moving something into Drupal core. ... Regarding external libraries it becomes even more special: Drupal core itself does not need this feature, so we would blindly add something to core that didn't undergo extensive testing.

I understand this, but Drupal Core has many faces in practice. Drupal-Core-the-standalone-CMS indeed wouldn't need it, but I think there is certainly place for it in Drupal-Core-the-webdevelopment-framework or Drupal-Core-doesnt-make-sense-without-contrib-stuff ;).
Regarding the testing, maybe it's not very straightforward, but it should be possible to cover it with simpletest?

Maybe I see it too small (e.g. the 20-40 LOC estimate above), but my point is that I would prefer to fork such a small amount of code over depending on another module.

Magnity’s picture

Surely this is a pro to the idea: Modules can implement 'libraries' to whatever level they want.
1 - Simply using sites/all/libraries to store the code
2 - Forking that function
3 - Relying on 'Libraries API'

All of these will be beneficial, and I think that we'll find that if more modules start to use sites/all/libraries, then more will then think about using the central 'Libraries API' to manage them too.

As an aside though: i'd much prefer functions to be named libraries_function_name() rather than drupal_function_name.

As previously said though, this discussion should probably move on now. It is no longer simply applicable to WYSIWYG API. Is there a particular g.d.o group that would be appropriate?

markus_petrux’s picture

If it ends up being 20 lines of code or 200 depends greatly on the experience collected while as much modules reuse the same pattern, and this can only be assured if the API is centralized. I think. Otherwise, it would pretty difficult to catch up little variations introduced by one module author or another.

One thing that could be introduced here is that your module would not have to store the location of the library in its own settings, just like it uses drupal_get_path(), and that may mean some kind of caching for the whole library could be needed to prevent from scanning the file system every now and then.

sun’s picture

@soxofaan: I think we are not on the same line. As mentioned in #13, I envision a bit more than a simple function that scans a few directories for sub-directories. For me, it roughly boils down to:

  • Detection: Identify whether a library exists (is installed).
  • UX: Guide in installing a library.
  • Compatibility: Determine, which version of a library is installed and whether that version is supported.
  • Negotiation: Inform about which features are supported by a library. (may or may not be handled by the API, but the consumer instead)
  • Dependencies: Properly handle interdependencies between external libraries.

@Magnity: I'd prefer using the issue queue of Libraries API to discuss. g.d.o has no project issue link filter and a weird subscriptions system. All of this is very actionable, so it would not make sense to spread discussion to other places and create concrete issues (that can be worked on) separately.

sun’s picture

@markus_petrux: Added caching as issue: http://drupal.org/project/issues/libraries

soxofaan’s picture

At sun in #47: ok, I see your bigger picture now and agree that's too much for core. That stuff needs to mature in contrib for sure.

But what about this little compromise (at the risk of going bikeshedding again ;) :

  • one small function drupal_get_library_path($library_name) (like there is drupal_get_path()) in Drupal core that offers basic (uncached) recursive searching of a (third party) library. This would be the "Detection" bullet point of #47.
  • contrib module "libraries" that builds on it and offers all the advanced stuff (the other bullet points, like UX, negotiation, caching etc)

That way modules that don't want the extra dependency of an extra module can use drupal_get_library_path() and take care of the desired features (like caching) themselves.
And modules that require the more advanced stuff (like Wysiwyg API) can depend on the "libraries" module.

The advantage of drupal_get_library_path($library_name) in core is that this puts the library location convention in core, which makes it easier to enforce promote towards users and module developers.

sun’s picture

one small function drupal_get_library_path($library_name) (like there is drupal_get_path()) in Drupal core that offers basic (uncached) recursive searching of a (third party) library.

If you scroll up to #13 you will identify the flaw in this proposal. recursive is, as of now, out of question. Potential solutions to be discussed in #465908: Scan: Name clashes. Leaving us with: There is no point in moving a function into core that returns the sub-directories of a directory (oversimplified).

And modules that require the more advanced stuff (like Wysiwyg API) can depend on the "libraries" module.

I do not intend to move advanced stuff into Libraries API. We are talking about API functions that allow modules to integrate external libraries in a way that tries to prevent errors and support requests in the first place.

Comparison: sites/all/libraries is like sites/all/modules. If your module ships with a sub-module, and you really want to make that installation hard, then just don't create an .info file, so it doesn't show up on admin/build/modules. Instead, let the user figure out that this sub-module only works on Drupal 5.x running on PHP5 and has to be enabled on admin/settings/yourmodule/submodule.

dkruglyak’s picture

FYI - moving TinyMCE to your libraries directory (per latest code) breaks 3rd party modules that depend on existing directory structure, such as img_assist.

You need a plan to either make them compatible on your end or get every other module maintainer make the needed fixes.

rickvug’s picture

This is an excellent idea. If this does get implemented, please don't overlook support for a libraries directory at the root level (/libraries). The use case is that Drupal distros want to place all standard code outside of the /sites directory, which may not even exist in their base install. This pattern would be consistent with the modules and themes directories.

sun’s picture

@rickvug: That is already taken care of. However, not at the root level, but as usual, install profiles can ship with libraries as well - just like they can ship with modules and themes.

Next issue, major issue: #469530: TinyMCE image assist popup not redirecting to new installation folder 'libraries'

Summary: Image Assist looks for an editor library in the wrong directory. Aside from the fact that Image Assist shouldn't need to do this at all, we need to solve that scenario somehow. Of course, committing this path change to the current 2.x version of Wysiwyg probably wasn't a good idea in the first place... anyway.

It seems we need to adapt an API scheme like Views/CTools does. Without that, other modules cannot react on API changes, which sucks.

sun’s picture

Status: Active » Needs review
FileSize
3.44 KB
7.65 KB

Kick-start. Moving essentials to Libraries module.

markus_petrux’s picture

Sweet. Here's a few comments:

1) Bug? In function libraries_get_path(), when !isset($libraries[$library]), then the $base_path argument is not taken into account.

2) Question/suggestion: In Wysiwyg API patch... wouldn't it be easier/faster to simply call libraries_get_path() rather than using module_invoke('libraries', 'get_path', ...)?

3) Would you mind committing stuff in libraries project? That way we can start using it now, for Wysiwyg API, but also easier to involve other module authors.

sun’s picture

1) Good catch! :)

2) In general, yes. However, in this special case, we are adding a module dependency on sites where Wysiwyg API is already installed. Without module_invoke(), users will get very ugly WSODs without having a clue about what could have gone wrong. We can replace those invokes with direct function calls at a later point in time.

3) I've committed the patch for Libraries as is now. But please let me clarify that the code really was a first kick-start only (copy & paste & rename from Wysiwyg). I'm still tinkering about the overall design. Already clear is that one of the next steps needs to introduce a hook like hook_views_api(), allowing other modules (like Wysiwyg) to expose which API version of Libraries they are compatible with.

4) I have no idea whether we should continue to commit to Wysiwyg 2.x. Originally, I created 2.x-alpha1 only to clarify for users that it's safe and more stable to use. I have no interest in ending up at version 8.x like OG though. However, we will soon also add dependencies on CTools and jQuery UI modules for other issues in the queue. Perhaps we should add all those dependencies in one fell swoop, so users updating to 2.x-alpha2 will only have to deal with this mess once.

markus_petrux’s picture

Agreed that messing with all these changes would be better just once for all. But there may be changes that are not so mature as this one, maybe.

One possible way to help during the transition to libraries API: 1) in wysiwyg_load_includes('editors', ...) check for module_exists('libraries'). If false, then return an empty array, and maybe display a drupal_set_message() warning and/or record a notice to watchdog. 2) Implement hook_requirements to provide a notice to site administrators they need to install libraries module.

markus_petrux’s picture

Reported the libraries issue here: ;-)

- #480440: $base_path argument is not taken into account for default location of library

So, now we have libraries integration with Wysiwyg API that probably needs work. :P

markus_petrux’s picture

Status: Needs review » Reviewed & tested by the community

Well, I installed the libraries module, and then tested the patch to Wysiwyg API in #54. I'm using TinyMCE 3 only, but it worked like a charm here, so I would say this is RTBC.

Thank you very much. The libraries module approach allowed me to get rid of a hack I've been maintaining for a lot of time to be able to place the TinyMCE library off the Wysiwyg API module directory. :-D

I think you could go ahead and create a release for the libraries module. Even if it was just a DEV release, it would probably help others test the patch here.

sun’s picture

Committing this library location change to 2.x was probably major mistake. However, since many people are already using 2.x, I will not revert that change.

I will create a new 3.x branch for this patch. Adding Libraries API as dependency. But also adding further dependencies, namely: CTools and jQuery UI. That will allow us to move forward with the rewrite of Wysiwyg API.

Especially regarding jQuery UI, I plan to create a new branch that equally implements Libraries API - so there is much to do; any contributions (issues, feedback, patches, testing) appreciated!

markus_petrux’s picture

Ugh, I see. This is complex situation where Drupal itself doesn't help much when adding module requirements when a module is already installed. :( ...not sure if using hook_requirements would help a little here, though. Maybe you could explore this possibility and that could save you from creating the 3 branch for Wysiwyg API.

I celebrate you plan on using Libraries API with jQuery UI. Let's see where this new libraries consumer may lead us. I'm also in need of jQuery UI, so I'll try to help.

naught101’s picture

Slightly off topic, but #496204: Configurable font storage location (eg. /usr/share/fonts)

Perhaps there should be a proposal to include ALL third-party (fourth-party?) requirements of modules (ie. anything that's not written specifically in /sites//libraries/...

sun’s picture

Thanks for that pointer. Not really off-topic. Rather a very interesting use-case.

Not really a library in the context of an application, but a repository of passive files that may be used by several modules. Storing those outside of the module folder and potentially in sites/all/libraries is definitely better.

However, in #465908: Scan: Name clashes, we are discussing how to prevent name clashes between libraries. That is, with regard to this example: who says that there cannot be a JavaScript or PHP library called "fonts"? Not sure how to solve that yet, but it's possible that we have to put libraries into some kind of "meta" category folders. To be discussed over there.

naught101’s picture

Perhaps it could be split into sites/all/libraries and sites/all/resources? Then again it's more cluttered, and who would name their package "fonts"?

Owen Barton’s picture

Subscribe

sun’s picture

Issue tags: +Libraries

Tagging.

anarcat’s picture

So what are we waiting for here? Why isn't this in yet? Blocked by an eventual Libraries release?

(Oh, and subscribing, great work :)

sun’s picture

+++ wysiwyg.info	1 Jun 2009 18:19:06 -0000
@@ -2,4 +2,5 @@
+dependencies[] = libraries

The new dependencies are already in HEAD now - so this patch needs to be re-rolled.

Note that there are some more editor include files in the meantime, which probably contain some more/new instances of wysiwyg_get_path().

This review is powered by Dreditor.

ezyang’s picture

Hello! I recently had users of one of my modules asking that I use the Libraries API to organize my module code. However, this bug--which appears to be the canonical source of information--has too much clutter for me to tell, as a module maintainer, what I should be doing. Will there be docs on how to use this posted at some point?

mparker17’s picture

Subscribe! Sounds awesome!

Stuart Greenfield’s picture

Subscribe.

irakli’s picture

@ezyang et al.,

documentation page created, so people don't need to scan through this long issue discussion: http://drupal.org/node/735160

Please feel free to edit, in case I missed something.

Cheers and thanks for a great effort.

sun’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev
Status: Reviewed & tested by the community » Needs work

Libraries API for D7 allows modules to implement hook_library_info() now, which has been primarily modeled after Wysiwyg's hook_editor(). Which means that we need to revamp this patch.

ogi’s picture

subscribe

batigol’s picture

Follow

naught101’s picture

batigol: wtf? Just press the "follow" button at the top of the page!

treksler’s picture

naught101: no, the WTF is that the follow button is at the top of the page, not where you need it, at the bottom

philbar’s picture

Issue summary: View changes
Status: Needs work » Closed (fixed)