Create a means to mark an asset library as deprecated in a *.libraries.yml file

Looks very promising to me, nothing to complain about in the patch.

This seems like a really good idea.

+++ b/core/core.libraries.yml
@@ -666,6 +666,7 @@ jquery.ui.effects.scale:
+  deprecation-message: "jquery.ui.effects.scale is deprecated in drupal:8.8.0 and will be removed in drupal:9.0.0. Require jQuery UI as an explicit dependency or create a pure javascript solution. See https://www.drupal.org/node/3064015."

I think we should go with what we do in the container. I.e use the key deprecated - less to have to remember and learn. See

  router.matcher.final_matcher:
    class: Drupal\Core\Routing\UrlMatcher
    arguments: ['@path.current']
    deprecated: The "%service_id%" service is deprecated. You should use the 'router.no_access_checks' service instead.

And maybe we could do something %library_name% or %library_id% too

Let's do both of those, although went for '%library%' for the placeholder.

Mostly looks great. Some minor nits:

1. +++ b/core/core.libraries.yml
   @@ -666,6 +666,7 @@ jquery.ui.effects.scale:
   +  deprecated: "%library% is deprecated in drupal:8.8.0 and will be removed in drupal:9.0.0. Require jQuery UI as an explicit dependency or create a pure javascript solution. See https://www.drupal.org/node/3064015."
   

   Core seems to use "JavaScript" consistently, not "javascript".

   Also, coder is now complaining about the formatting of deprecation messages:

   "The text '@deprecated %s' does not match the standard format: @deprecated in %%deprecation-version%% and is removed from %%removal-version%% . %%extra-info%%.";

   Finally, I don't think we want that ending period after the URL in @see.

   So, I think we want:
   "%library% is deprecated in drupal:8.8.0 and is removed in drupal:9.0.0. Require jQuery UI as an explicit dependency or create a pure JavaScript solution. See https://www.drupal.org/node/3064015"

2. +++ b/core/tests/Drupal/KernelTests/Core/Asset/LegacyLibraryDiscoveryTest.php
   @@ -0,0 +1,39 @@
   +   * @expectedDeprecation jquery.ui.effects.scale is deprecated in drupal:8.8.0 and will be removed in drupal:9.0.0. Require jQuery UI as an explicit dependency or create a pure javascript solution. See https://www.drupal.org/node/3064015.
   

   This will have to be changed to match whatever happens with point 1.

3. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
   @@ -50,6 +50,13 @@ class LibraryDiscoveryTest extends UnitTestCase {
   +      'deprecated' => '%library% is deprecated in drupal:8.8.0 and will be removed in Drupal 9.0.0.  Use the test_2 library instad. See https://www.example.com',
   

   s/will be removed/is removed/ per #1.
   s/Drupal 9.0.0/drupal:9.0.0/
   s/instad/instead/
   Also, inconsistent with two spaces after period or not. Should always use a single space.

4. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
   @@ -84,4 +90,28 @@ public function testGetLibrariesByExtension() {
   +   * @group legacy
   

   As above, should this also be in '@group Asset'?

5. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
   @@ -84,4 +90,28 @@ public function testGetLibrariesByExtension() {
   +   * @expectedDeprecation test_3 is deprecated in drupal:8.8.0 and will be removed in Drupal 9.0.0.  Use the test_2 library instad. See https://www.example.com
   

   Needs update to match point 3.

This fixes everything from #9 except point 4 which I wasn't sure about.

p.s. I updated the change record to match the current deprecation message and to fix a few grammar + formatting bugs.

p.p.s. Note to committers: @mikelutz wrote the bulk of this patch. @catch and I just did some tweaking. Sadly, d.o's credit heuristics list @mikelutz as the last author, but he should get 1st billing on this issue. Please modify the commit message accordingly. Thanks!

Oof. The new test needs @doesNotPerformAssertions after #3059332: Mark kernel tests that perform no assertions as risky

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
@@ -84,4 +90,28 @@ public function testGetLibrariesByExtension() {
+   * This test tests the mechanism for triggering an error from a library file.
+   * It is marked @group legacy to test for the deprecation message, but is not
+   * a legacy test and should not be removed in future major versions.
+   *
+   * @group legacy
+   *
+   * @expectedDeprecation test_3 is deprecated in drupal:8.8.0 and is removed in drupal:9.0.0. Use the test_2 library instead. See https://www.example.com

This can be done in another slightly cleaner way. We can use a custom error handler in the test and catch the deprecation here ourselves. And test that. Then we don't have to mark this test @group legacy when it is not.

See \Drupal\KernelTests\Core\Database\QueryTest::testConditionOperatorArgumentsSQLInjection for an example of a custom error handler in a test.

Or https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Con... for a specific example for deprecations.

1. I was going to make the same remark as @alexpott already made in #5 😀

2. +++ b/core/core.libraries.yml
   @@ -666,6 +666,7 @@ jquery.ui.effects.scale:
   +  deprecated: "%library% is deprecated in drupal:8.8.0 and is removed in drupal:9.0.0. Require jQuery UI as an explicit dependency or create a pure JavaScript solution. See https://www.drupal.org/node/3064015"
   

   1. This is close, but not exactly like the pattern we use for the deprecated key in
      

      *.service.yml</code > files.
      
      I think it'd be good to follow the exact same pattern.
      
      They all look like this:
      <code>
      deprecated: The "%service_id%" service is deprecated. You should … instead.
      

      We can adopt at least the prefix here.

   2. For consistency we should use %library_id%, not %library%.
   3. Also: "is removed" should be "will be removed"

3. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
   @@ -84,4 +90,28 @@ public function testGetLibrariesByExtension() {
   +   * Test getting a library by name.
   

   Übernit: s/Test/Tests/

#15++

This reroll addresses that.

On top of that, the current method name causes this:

Using the "testLegacy" prefix to mark tests as legacy is deprecated since version 3.3 and will be removed in 4.0. Use the "@group legacy" notation instead to add the test to the legacy group.

So also renamed it.

Improved issue summary & title.

I wondered whether it is okay to only update getLibraryByName() — what about \Drupal\Core\Asset\LibraryDiscovery::getLibrariesByExtension()?

Turns out this is only called by tests, at least in core. #2203411: Convert drupal_get_library() into a service introduced it. And even there it was only called by tests. Shall I open a follow-up to deprecate that method?

Wim ++ to followup, api & scope

The message needs one more review, rtbc++

@Wim Leers: thanks for picking this up and running with it!

Re: #16.2.3:

"is removed" should be "will be removed"

Tell that to coder. :/ See #2908391: Add a rule for expected format of @deprecated and @trigger_error() text. The version of coder that's installed in vendor to do our code sniff testing is expecting this:

        ...
        preg_match('/in (.+) and is removed from (?U)(.+)\. (.+)$/', $depText, $matches);
        // There should be 4 items in $matches: 0 is full text, 1 = in-version,
        // 2 = removal-version, 3 = extra-info.
        if (count($matches) !== 4) {
            $error = "The text '@deprecated %s' does not match the standard format: @deprecated in %%deprecation-version%% and is removed from %%removal-version%%. %%extra-info%%.";
            $phpcsFile->addError($error, $stackPtr, 'IncorrectTextLayout', [$depText]);
        } else {  // [sic] Hilarious that coder itself doesn't strictly follow Drupal's coding standards.
        ...

See also https://www.drupal.org/core/deprecation and #3024461: Adopt consistent deprecation format for core and contrib deprecation messages.

Personally, I agree "will be removed in" is better than "is removed from", but apparently that's not what @xjm and @jonathan1055 went with (over the objections of @alexpott, it seems). Anyway, "is removed from" is now what's documented and enforced by coder, so I think we're stuck with it. That's why I raised it at #9.1 and #9.3.

Therefore, I believe we have to change your (otherwise nice) deprecation messages to be like this:

The "%library_id%" asset library is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Require jQuery UI as an explicit dependency or create a pure JavaScript solution. See https://www.drupal.org/node/3064015

👍🙏✅

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
@@ -56,7 +56,13 @@ public function getLibrariesByExtension($extension) {
+    if (isset($extension[$name]['deprecated'])) {
+      @trigger_error(str_replace('%library_id%', $name, $extension[$name]['deprecated']), E_USER_DEPRECATED);
+    }

Should we also mention the extension that the library belongs to? Having only the name of the library isn't specific enough because a library with the same name could be provided by multiple extensions.

+1, will do!

Done.

@lauriii: +1 on #24.

#26 looks great to me. Thanks, @Wim Leers!

No further concerns. I've got some fingerprints on this patch, but I still feel safe to RTBC.

Thanks!
-Derek

p.s. Reminder to core committers: I still think @mikelutz should get "first billing" on the commit message for this issue. Not that it really matters, but I think it's a kind thing to do. I believe the accurate order should be:

git commit -m 'Issue #3064017 by mikelutz, Wim Leers, dww, catch: Create a means to mark an asset library as deprecated in a *.libraries.yml file'

+++ b/core/tests/Drupal/KernelTests/Core/Asset/LegacyLibraryDiscoveryTest.php
@@ -0,0 +1,40 @@
+class LegacyLibraryDiscoveryTest extends KernelTestBase {

Deprecating jquery.ui.effects.scale feels like outside of the scope of this issue. I think it would be better to handle as part of https://www.drupal.org/project/drupal/issues/3051352.

Another concern I have related to this is that jQuery UI is deprecated from Drupal 9.0.0, and we probably want this test coverage to stay in place after Drupal 9.0.0 has shipped. Any thoughts on making this test depend on a test module or theme that has a deprecated library instead?

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
@@ -50,6 +50,13 @@ class LibraryDiscoveryTest extends UnitTestCase {
     ],
+    'test_3' => [
+      'js' => [
+        'baz.js' => [],
+      ],
+      'css' => [],
+      'deprecated' => 'The "%library_id%" asset library is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use the test_2 library instead. See https://www.example.com',
+    ],
   ];
 

This hunk is enough for general test coverage of the feature, we don't need a test module.

The only issue with doing the actual scale deprecation here is that #3051352: [Plan] Remove unused jQuery UI components and replace with a suite of contrib packages for the continuous upgrade path is still being discussed.

#30: hah, that's exactly what I was about to reply with :)

So to resolve #28 (which I agree with by the way!), I just omitted all actual asset library hunks from the patch.

+1, tests are still running, but I'm happy with the current patch.

So the work here doesn't get lost (other than hidden in an interdiff), I posted the removed hunks to a proof-of-concept patch at #3051352-39: [Plan] Remove unused jQuery UI components and replace with a suite of contrib packages for the continuous upgrade path so folks can expand on it over there.

Meanwhile, +1 to RTBC in here.

Cheers,
-Derek

Committed 0d82715 and pushed to 8.8.x. Thanks!

Polishing the release note a bit to explain why this is a change that people should be aware of.

Also crediting @pameeela for helping improve the release note.

Typo.