Convert drupal_get_library() into a service and:

  1. Inject all dependencies.
  2. Consider to use a single cache item for all extensions.
  3. Resolve the @todo of determining whether $extension is a module or theme.
  4. Resolve the @todo of determining the version of contributed and custom modules/themes.
  5. Introduce proper unit tests for drupal_get_library() (there are none yet).
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

dawehner’s picture

Assigned: Unassigned » dawehner

Let's try it.

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Active » Needs review
FileSize
19.05 KB

Let's post the work of tonight, there are not unit tests yet.

Status: Needs review » Needs work

The last submitted patch, 3: libraries-2203411-3.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit
FileSize
38.63 KB
8.66 KB

This should cover at least somehow the basic cases for this service.

Status: Needs review » Needs work

The last submitted patch, 5: library_discovery-2203411-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.3 KB
482 bytes

ups.

Status: Needs review » Needs work

The last submitted patch, 7: library_discovery-2203411-7.patch, failed testing.

sun’s picture

The last submitted patch, 7: library_discovery-2203411-7.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.37 KB
1.13 KB

The last failure seemed to be random.

Status: Needs review » Needs work

The last submitted patch, 11: drupal_get_library-2203411-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
38.37 KB

embarrassing, there is nothing else to describe it.

dawehner’s picture

FileSize
1.62 KB

.

dawehner’s picture

Wim Leers’s picture

Status: Needs review » Needs work
FileSize
39.4 KB
6.62 KB

:)

  1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
    @@ -0,0 +1,273 @@
    +  protected function ensureLibraryInformation($extension) {
    ...
    +  protected function retrieveFromCache($extension) {
    ...
    +  protected function setCache($extension, array $information) {
    ...
    +  protected function buildLibrariesByExtension($extension) {
    ...
    +  protected function fileValidUri($source) {
    ...
    +  protected function parseLibraryInfo($extension, $library_file) {
    

    Missing docs.

  2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
    @@ -0,0 +1,273 @@
    +        throw new \RuntimeException(sprintf("Incomplete library definition for '%s' in %s", $id, $library_file));
    

    HURRAY! Explicit failure!

  3. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
    @@ -0,0 +1,408 @@
    +      'description' => '',
    

    Missing description.

  4. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
    @@ -0,0 +1,408 @@
    +   * @expectedException \RuntimeException
    

    Shouldn't we have a more specific exception for this?

  5. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
    @@ -0,0 +1,408 @@
    +   * @expectedException \RuntimeException
    +   * @expectedExceptionMessage Incomplete library definition for 'example' in core/tests/Drupal/Tests/Core/Asset/library_test_files/example_module_missing_information.libraries.yml
    

    Another specific exception would be nice?

  6. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
    @@ -0,0 +1,408 @@
    +class TestLibraryDiscovery extends LibraryDiscovery {
    +
    +  protected $paths;
    +
    +  protected $validUris;
    +
    +  protected function drupalGetPath($type, $name) {
    +    return isset($this->paths[$type][$name]) ? $this->paths[$type][$name] : NULL;
    +  }
    +
    +  public function setPaths($type, $name, $path) {
    +    $this->paths[$type][$name] = $path;
    +  }
    +
    +  protected function fileValidUri($source) {
    +    return isset($this->validUris[$source]) ? $this->validUris[$source] : FALSE;
    +  }
    +
    +  public function setFileValidUri($source, $valid) {
    +    $this->validUris[$source] = $valid;
    +  }
    +
    +}
    

    This appears to be dead code?

  7. +++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/css_js_settings.libraries.yml
    --- /dev/null
    +++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/css_weights.libraries.yml
    

    Great test!

  8. +++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/dependencies.libraries.yml
    @@ -0,0 +1,9 @@
    +    -
    +      - example_theme
    +      - example
    

    This looks bizarre… If this is intentional, it could definitely use a comment.

dawehner’s picture

Status: Needs work » Needs review
FileSize
35.78 KB
41.37 KB
7.88 KB

Thank you for the review and all the grammar/case sensitivity fixes.

Missing docs.

Okay, here are some:

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
@@ -0,0 +1,408 @@
+      'description' => '',
Missing description.

People started to not put any pointless description onto phpunit tests.

This appears to be dead code?

This is used by the test to mock its external dependencies.

This looks bizarre… If this is intentional, it could definitely use a comment.

Do we have a better syntax for such an subarray? The idea is (at least for now), to support both the "external/example_external" and the other syntax. Maybe we could already drop the functionality in this issue.

Another specific exception would be nice?

Came up with two custom exceptions. I am not sure about the remaining UnexpectedValueException one, as it provides somehow enough context already.

damiankloip’s picture

  1. +++ b/core/lib/Drupal/Core/Asset/Exception/IncompleteLibraryDefinitionException.php
    @@ -0,0 +1,15 @@
    +class IncompleteLibraryDefinitionException extends \RuntimeException {
    
    +++ b/core/lib/Drupal/Core/Asset/Exception/InvalidLibraryFileException.php
    @@ -0,0 +1,15 @@
    +class InvalidLibraryFileException extends \RunTimeException {
    

    I like these.

  2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
    @@ -0,0 +1,316 @@
    +  protected function retrieveFromCache($extension) {
    ...
    +  protected function setCache($extension, array $information) {
    

    Seems like a naming mismatch. Can we just go with setCache/getCache. This is more inline with naming elsewhere IMO.

  3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
    @@ -0,0 +1,316 @@
    +      // @todo Convert all uses of #attached[library][]=array('provider','name')
    

    Nice. This will be a good tidy up. Is there an issue for it yet?

  4. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
    @@ -0,0 +1,316 @@
    +    return drupal_get_path($type, $name);
    ...
    +    return file_valid_uri($source);
    

    Not for this issue at all. Would be nice to have some non procedural file stuff. Seems a long way off though :/

dawehner’s picture

Nice. This will be a good tidy up. Is there an issue for it yet?

Not sure, as I just copy and pasted the stuff.

Not for this issue at all. Would be nice to have some non procedural file stuff. Seems a long way off though :/

Wait, really? I guess at least for drupal_get_path() we will have a solution with the extension subsystem.

dawehner’s picture

FileSize
47.04 KB

Thank you for the review damian! Sadly the interdiff failed :(

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
    @@ -0,0 +1,316 @@
    +    if ($extension === 'core') {
    +      $path = 'core';
    +      $extension_type = 'core';
    +    }
    ...
    +          if (!is_array($options)) {
    +            $options = array();
    +          }
    ...
    +          // Determine the file asset URI.
    +          else {
    +            if ($source[0] === '/') {
    +              // An absolute path maps to DRUPAL_ROOT / base_path().
    +              if ($source[1] !== '/') {
    +                $options['data'] = substr($source, 1);
    +              }
    +              // A protocol-free URI (e.g., //cdn.com/example.js) is external.
    +              else {
    +                $options['type'] = 'external';
    +                $options['data'] = $source;
    +              }
    +            }
    +            // A stream wrapper URI (e.g., public://generated_js/example.js).
    +            elseif ($this->fileValidUri($source)) {
    +              $options['data'] = $source;
    +            }
    +            // By default, file paths are relative to the registering extension.
    +            else {
    +              $options['data'] = $path . '/' . $source;
    +            }
    +          }
    

    All of this except the final else (with $path . '/' . $source) are not covered by the unit tests.
    Do we care about adding some extra tests for this?

  2. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
    @@ -0,0 +1,411 @@
    +if (!defined('CSS_AGGREGATE_DEFAULT')) {
    +  define('CSS_AGGREGATE_DEFAULT', 0);
    +  define('CSS_AGGREGATE_THEME', 100);
    +  define('CSS_BASE', -200);
    

    Oh that's way smarter than separate !defined checks, +1

  3. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
    @@ -0,0 +1,411 @@
    +  /**
    +   * Tests that basic functionality works for getLibrariesByExtension.
    +   *
    +   * @covers ::getLibrariesByExtension()
    +   */
    +  public function testGetLibrariesByExtensionSimple() {
    ...
    +  /**
    +   * Tests that a theme can be used instead of a module.
    +   */
    +  public function testGetLibraryByNameWithTheme() {
    

    I love the usage of @covers here, but am sad about missing it on the rest.

dawehner’s picture

FileSize
50.72 KB
8.96 KB

I love the usage of @covers here, but am sad about missing it on the rest.

Added some to the rest of them.

All of this except the final else (with $path . '/' . $source) are not covered by the unit tests.
Do we care about adding some extra tests for this?

Let's just do it.

sun’s picture

Status: Needs review » Needs work

#2203407: Replace #attached library array values with provider-namespaced strings has landed, so this needs a re-roll now.

Note that the former support/handling of array('provider', 'lib-name') has been removed entirely by that patch; it's always provider/lib-name now.

dawehner’s picture

Status: Needs work » Needs review
FileSize
49.48 KB
2.2 KB

Rerolled. The interdiff are the changes after the rerole.

Status: Needs review » Needs work

The last submitted patch, 24: library_discovery-2203411-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
49.49 KB
793 bytes

Let's see

Status: Needs review » Needs work

The last submitted patch, 26: library_discovery-2203411-26.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
49.45 KB
1.56 KB

Unit tests are not everything.

dawehner’s picture

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Awesome work!

The last submitted patch, 16: library_discovery-2203411-16.patch, failed testing.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
49.47 KB
2.42 KB

#2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses broke this.

Reroll + fix super minor nitpicks (for which there is an interdiff).

dawehner’s picture

Thank you for the rerole and the nitpicks!

diff --git a/core/tests/Drupal/Tests/Core/Asset/library_test_files/invalid_file.libraries.yml b/core/tests/Drupal/Tests/Core/Asset/library_test_files/invalid_file.libraries.yml
index b3cebbb..cb20993 100644
--- a/core/tests/Drupal/Tests/Core/Asset/library_test_files/invalid_file.libraries.yml
+++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/invalid_file.libraries.yml
@@ -1,2 +1,2 @@
-	example:
+example:
   -=--##;'~ test

Are you sure this still passes? I remember that I needed the tab to get a failure in the unit test.

Status: Needs review » Needs work

The last submitted patch, 32: library_discovery-2203411-32.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
49.48 KB
458 bytes

Let's go with a slightly simpler example.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

back to RTBC, this will pass.

Wim Leers’s picture

#33: sorry, git complained, I figured it was the actual value that was going to trigger a fail, apparently it wasn't :( Thanks for the simpler example in #35!

RTBC +1

moshe weitzman’s picture

Priority: Normal » Major
Issue tags: +Performance

This helps get rid of several cache gets. Up priority.

damiankloip’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work

Started reviewing this but I'm almost out of battery so posting what I have so far (which wasn't far in)..

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
@@ -0,0 +1,316 @@
+  public function __construct(CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler) {

ThemeHandler isn't documented as a property, isn't set in __construct(), and never gets used.

catch’s picture

Still partial review, but:

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
@@ -0,0 +1,316 @@
+    $this->moduleHandler->alter('library_info', $this->libraries[$extension], $extension);

Themes can implement hook_library_info_alter(), but the current theme isn't taken into account in the cache key. Looks like a pre-existing problem though.

damiankloip’s picture

#41, yeah, if anything I think we could roll that into the work we will do after this patch in #2223143: Consolidate library extension caches into a single cache entry?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
49.33 KB
1.35 KB

Removed the theme handler from the LibraryDiscovery constructor.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Haven't found anything to object. Tests are there to make sure every single scenario is tested so I think it is good to go RTBC

dawehner’s picture

Really good catch!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 43: 2203411-43.patch, failed testing.

damiankloip’s picture

43: 2203411-43.patch queued for re-testing.

jibran’s picture

Status: Needs work » Reviewed & tested by the community

:/

dawehner’s picture

Meh!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit 63375a5 on 8.x by catch:
    Issue #2203411 by dawehner, Wim Leers, damiankloip: Convert...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks!

damiankloip’s picture

Yes, nice work sun, dawehner!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.