Separate installed from uninstalled libraries + display unregistered (but available) libraries

Yeah, I agree this is somewhat problematic. The problem is that we currently don't have a way to know which libraries are required and which are merely optional.

Do you have any suggestions on how we could improve the wording / labeling? Maybe it makes sense to completely separate installed and uninstalled libraries to make the uninstalled ones less prominent?

I like the new UI, I think we should pursue that. Retitling to that effect.

Thanks!!!

Some thoughts on the patch:

-  $header = array(t('Name'), t('Status'), t('Installed version'), t('Provider'), t('Links'));
-  $rows = array();
+  $libraries_header = array(t('Name'), t('Version'), t('Provider'), t('Links'));

I agree with changing "Installed version" to "Version". ++
I think it makes sense to keep "Status" for the not-installed table, so let's have separate $header_installed and $header_error variables.

+  $ok_rows = array();
+  $not_ok_rows = array();

Similar to the above, I would suggest $rows_installed and $rows_error.

+  $requirements = module_invoke_all('requirements', 'runtime');

We should not do that. We should solve this problem properly at some point and allow modules to declare dependencies on libraries. At that point we could implement hook_requirements() for those modules and also display this information in the report, but let's not do it this way around. Please just remove this for now and see you in #1148246: Implement hook_requirements() to list libraries ;-)

+  $directory_libraries = array_filter(glob('sites/all/libraries/*'), 'is_dir');

Please just use libraries_get_libraries(), that should be sufficient.

+    if ($library['installed'] == t('OK')) {

Use if ($library['installed']) instead, it is a boolean.

+      $ok_rows[] = array(
+        'data' => array(
+          l($library['name'], 'admin/reports/libraries/' . $machine_name),
+          (isset($library['version']) ? $library['version'] : ''),
+          libraries_admin_get_provider_with_type($library),
+          implode(' | ', $actions),
+        ),
+        'class' => (array('ok')),
+      );
+    }
+    else {
+      $not_ok_rows[] = array(
+        'data' => array(
+          l($library['name'], 'admin/reports/libraries/' . $machine_name),
+          (isset($library['version']) ? $library['version'] : ''),
+          libraries_admin_get_provider_with_type($library),
+          implode(' | ', $actions),
+        ),
+      );

Instead of duplicating it, let's just build up a $row variable once and then either stick it in $rows_installed or in $rows_error depending on the status.

-    $library['error message'] = t('The %library library could not be found.', array(
-      '%library' => $library['name'],
-    ));
+    if ($library['library path'] === FALSE) {
+      $library['error message'] = t('The %library library is inactive, unused, or could not be found.', array(
+        '%library' => $library['name'],
+      ));
+    }
+    else {
+      $library['error message'] = t('The %library library could not be found.', array(
+        '%library' => $library['name'],
+      ));
+    }

Why is this necessary? Please revert this in case it is not necessary for this patch.

Since were already overhauling this, I also found a couple things which are wrong with the current code (i.e. my fault, not yours ;-)), can you fix them as well, as part of this patch? That would be awesome:

    if ($library['vendor url']) {
      $links[] = l('Homepage', $library['vendor url']);
    }
    if ($library['download url']) {
      $links[] = l('Download', $library['download url']);
    }

Please wrap 'Homepage' and 'Download' in t().

      case 'missing dependency':
        $form['instructions']['instruction']['#markup'] = t('There a missing dependency in your configuration that prevent this library to work properly.') . '<br>';
        break;

      case 'incompatible dependency':
        $form['instructions']['instruction']['#markup'] = t('There an incompatible dependency in your configuration that prevent this library to work properly.') . '<br>';
        break;

Those are not proper sentences, please change that to:

      case 'missing dependency':
        $form['instructions']['instruction']['#markup'] = t('There is a missing dependency in your configuration that prevents this library from working properly.') . '<br>';
        break;

      case 'incompatible dependency':
        $form['instructions']['instruction']['#markup'] = t('There is an incompatible dependency in your configuration that prevents this library from working properly.') . '<br>';
        break;

Hmm... I disagree, to be honest. The library, can - in fact - not be found. The thing is that we currently make it seem as though that were a problem for the site operation. That is the problem, I think. So we should make it seem less like "This is definitely a problem" and rather make it seem like "This might be a problem, but maybe you're doing just fine." I don't know how to do that, specifically, I think the separation is a step towards that, but I think changing the error message is not the right thing.

Hmm... let's see I'm still not quite convinced :-) Specifically this:

If $library['library path'] === FALSE, the situation is ambiguous. Returning FALSE could mean that the library is missing, or it could be a situation like Views Ticker, where the library is not being used.

This is incorrect. If $library['library path'] is set to FALSE it means that libraries_get_path() returned FALSE which is identical to saying that the library could not be found. The only other possibility is that it was set to FALSE upfront which is completely pointless and not supported.

Yay, I'm glad we're on the same page, now. :-)

The patch looks really close, thanks for sticking with it.

Some comments:

   $libraries = libraries_detect();
...
+  $dir_libraries = array();

I think we should go with $libraries_registered and $libraries_unregistered and explain the difference in a code comment above that. Something like:

libraries_get_libraries() searches for libraries that are actually available in one of the libraries directories in the filesystem. However, even if they are found they might not be installed correctly, if it is a wrong version of the library, for example. libraries_detect() will detect this for libraries that we have metadata for, so we prefer to use that. However, modules might also depend on Libraries API without providing library metadata and just using libraries_get_path(). Therefore we show those libraries that libraries_get_libraries() finds, but libraries_detect() does not know about in a separate table.

That's a little verbose, so feel free to improve on that, but something like that.

Also not 100% sure about "registered" and "unregistered" as variable names, maybe you have better suggestions?

+  $directory_libraries = libraries_get_libraries();
+  foreach ($directory_libraries as $directory_library) {
+    $name_split = explode('libraries/', $directory_library);
+    $dir_libraries[$name_split[1]] = array(
+      'name' => $name_split[1],
+      'library path' => $directory_library,
+    );
+  }

libraries_get_libraries() keys by library name (although apparently that is not properly documented :-() so I think we can drop the entire foreach.

+    if (!$library['installed']) {
+      $row['data'][] = drupal_ucfirst($library['error']);
+    }

I think this deserves a code comment. In fact, it would even be cool to put a comment like "Only show the status libraries with an error." above the declaration of the header variables, and then we can put a comment like "Only show the status for libraries with an error, see above." here. I think that makes it a bit more clear, what is happening.

+    // Filter libraries_detect entries from libraries_get_libraries values.
+    foreach ($dir_libraries as $dir_key => $dir_value) {
+      if ($library['library path'] === $dir_value['library path']) {
+        unset($dir_libraries[$dir_key]);
+      }
+    }

Because library machine names have to be unique I think we can simplify this to unset($libraries_registered[$library['machine name']]);

+    '#prefix' => '<br/><h2>',
+    '#markup' => t('Active libraries'),
+    '#suffix' => '</h2>',

I'm not super happy about putting the markup in #prefix and #suffix especially the <br> part, but I don't have any suggestion on how to improve that off the top of my head. So if you don't either I would be fine committing it this way, we can always improve in a follow-up. Just wanted to mention that regardless.

+  $header = array(t('Name'), t('Location'));

I would go with "Path" instead of "Location", we call it path everywhere else. Also I think it makes sense to put this declaration up to the top with the other headers. Then we have $header_installed, $header_error and $header_unregistered. Having written that, I was thinking whether it would make sense to split those three tables into three different functions, but probably we would have to refactor a bunch of more code, so let's consider that in a follow-up maybe (if at all).

+  if (count($rows)) {

Instead of only conditionally outputting the render array for the "unregistered" table, I think it's preferrable to do

$form['libraries']['unregistered'] = array(
  '#type' => 'table',
  ...
  '#access' => (bool) $libraries_unregistered,
);

I.e. always output the render array for the tables but hide it via #access if there are no rows. That way if people alter/extend this render array in some way, they can avoid isset() checks.

Again, thanks a lot for sticking with this. I really feel good about this now.

Thanks a lot for the patch!

I went ahead and committed it...

... but I couldn't help myself from adapting the wording and the table heading a bit. I also moved them into the table element itself with a #prefix. (Sorry for going back and forth on that a bit.) I think we should - in a follow-up - create a dedicated theme function that is like theme_table() but also takes a #title and a #description.

I went back to "Installed/Uninstalled/Unregistered" on the table headers. I still think that is more clear. But since I know you have a slightly different view on this marking this "Active" so we can further discuss UI/wording improvements if you want. But now the rest of the restructuring is in 8.x-2.x so further patches should be manageable more easily and I definitely think this is an improvement over previous 8.x-2.x.

If you think the current wording is fine, feel free to mark the issue fixed.

Attached interdiff shows my pre-commit changes.

Just committed a small follow-up that implements such a theme function mentioned in #19.

Still would love to hear your thoughts, @ron_s! :-)

Hmm... OK let's see.

This is why I initially added the $requirements = module_invoke_all('requirements', 'runtime'); table.

In the case of CKEditor that might just work, but in general this vastly incorrect. There is no defined structure for the keys that come out of hook_requirements(). They might just match any library machine name, but they also might not, and more importantly some completely different information might be portrayed there that has nothing to do with libraries. Therefore for me this is a complete no-go. I don't see how we can possibly get away with this. I didn't put it in such strong words earlier, because I didn't want to needlessly evoke a conflict, but I feel very strongly that this is incorrect, not only theoretically, but also practically a can of worms that we would be opening.

I believe "active", "inactive", and "other" are a better representation of these categories, while "installed", "uninstalled", and "unregistered" are less user-friendly.

Hmm, I actually disagree with this. I think specifically "installed" is pretty clear, because installing a library is similar to installing a module, specifically for people downloading stuff and FTP-ing it to their server. And registered vs. unregistered I can see being a little unintuitive, but I find "active" and "inactive" to be completely vague. And it's also not really correct. I.e. we have no idea if any library is "active" or not, maybe it's only active on certain pages, or under certain conditions. And maybe it's not being used at all. Again, this is not so much about technical correctness for me, but more about telling the user what is actually happening. And I don't feel that your terminology was doing a significantly better job at that than mine. And mine is consistent with the rest of the code, so I chose that.

Hmm.. marking this fixed due to lack of feedback.

Again @ron_s your effort on this and your input is highly appreciated, I hope this issue has not demotivated you from working on Libraries API in the future. If that is the case I sincerely apologize.

That's good to hear. Yeah, I think in the scope of this issue there is not much left to say. I just hope to see you in the queue again, the next you encounter an issue with Libraries API ;-) Thanks again!