Detect when libraries-override or libraries-extend is used on deprecated library

Added some tests and adjusted the deprecation messages to display the change record.

While I was working on this, I opened #3166553: Move libraries-extend and libraries-override processing to a more logical class too to make the processing easier to understand and more logical.

Moving back to NR since the fails are in the test only patch

Looks pretty good so far!

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
   @@ -136,6 +136,11 @@ protected function applyLibrariesExtend($extension, $library_name, $library_defi
   +          $library_deprecation = str_replace('%library_id%', "$extension/$library_name", $library_definition['deprecated']);
   

   Nice, I like how this is similar to service deprecation in Symfony.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
   @@ -136,6 +136,11 @@ protected function applyLibrariesExtend($extension, $library_name, $library_defi
   +          @trigger_error(sprintf('%s %s', $extend_message, $library_deprecation), E_USER_DEPRECATED);
   

   Is sprintf('%s %s') really necessary, we can just use concatenation?

3. +++ b/core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -208,6 +213,33 @@ public function testLibrariesExtend() {
   +    $expected_derecations = [
   

   Typo in "deprecations"

Thanks for the review @longwave!

#11.2 I don't have a strong preference on to which approach we should use. I changed this to double quotes but I'm fine with concatenation too.

#11.3 Fixed

Fixing 12 CS violations further.

1. +++ b/core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -208,6 +213,35 @@ public function testLibrariesExtend() {
   +    $expected_deprecations = [
   +      'Theme "theme_test" is overriding a deprecated library. The "theme_test/deprecated_library" asset library is deprecated in drupal:9.1.0 and is removed from drupal:9.0.0. Use another library instead. See https://www.example.com',
   +      'Theme "theme_test" is extending a deprecated library. The "theme_test/another_deprecated_library" asset library is deprecated in drupal:9.1.0 and is removed from drupal:9.0.0. Use another library instead. See https://www.example.com',
   +      'The "theme_test/deprecated_library" asset library is deprecated in drupal:9.1.0 and is removed from drupal:9.0.0. Use another library instead. See https://www.example.com',
   +      'The "theme_test/another_deprecated_library" asset library is deprecated in drupal:9.1.0 and is removed from drupal:9.0.0. Use another library instead. See https://www.example.com',
   +    ];
   +    $this->assertSame($expected_deprecations, $deprecations);
   

   Why do we need the error handler dance in the test, can't we use the @expectedDeprecation tag?

2. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryCollectorTest.php
   @@ -150,4 +166,95 @@ public function testDestruct() {
   +    catch (\ErrorException $e) {
   +      $this->assertSame('Theme "test" is extending a deprecated library. The "test/test_4" asset library is deprecated in drupal:9.1.0 and is removed from drupal:9.0.0. Use the test_3 library instead. See https://www.example.com', $e->getMessage());
   +    }
   

   Similar here, we have ExpectDeprecationTrait to use in unit tests.

Both, Symfony\Bridge\PhpUnit\ExpectDeprecationTrait and @expectedDeprecation requires adding the test to the @group legacy. I'm not sure we want that here because we are not testing deprecated code but our own APIs that allow deprecating code. Similar approach was taken in #3064017: Create a means to mark an asset library as deprecated in a *.libraries.yml file.

Hm, we have lots of tests that test deprecation implementations, that do use @group legacy with @expectedDeprecation, some examples:

core/tests/Drupal/FunctionalJavascriptTests/JavascriptDeprecationTest.php:   * @expectedDeprecation Javascript Deprecation: This function is deprecated for testing purposes.
core/tests/Drupal/FunctionalTests/Core/Test/PhpUnitBridgeTest.php:   * @expectedDeprecation This is the deprecation message for deprecation_test_function().
core/tests/Drupal/KernelTests/Core/Extension/ModuleHandlerDeprecatedHookTest.php:   * @expectedDeprecation The deprecated alter hook hook_deprecated_alter_alter() is implemented in these functions: deprecation_test_deprecated_alter_alter. Alter something else.
core/tests/Drupal/KernelTests/Core/Extension/ModuleHandlerDeprecatedHookTest.php:   * @expectedDeprecation The deprecated hook hook_deprecated_hook() is implemented in these functions: deprecation_test_deprecated_hook(). Use something else.
core/tests/Drupal/KernelTests/Core/Extension/ModuleHandlerDeprecatedHookTest.php:   * @expectedDeprecation The deprecated hook hook_deprecated_hook() is implemented in these functions: deprecation_test_deprecated_hook(). Use something else.
core/tests/Drupal/KernelTests/Core/Test/PhpUnitBridgeTest.php:   * @expectedDeprecation Drupal\deprecation_test\Deprecation\FixtureDeprecatedClass is deprecated.
core/tests/Drupal/KernelTests/Core/Test/PhpUnitBridgeTest.php:   * @expectedDeprecation This is the deprecation message for deprecation_test_function().
core/tests/Drupal/Tests/Core/Security/DoTrustedCallbackTraitTest.php:   * @expectedDeprecation Drupal\Tests\Core\Security\UntrustedObject::callback is not trusted
core/tests/Drupal/Tests/Core/Test/PhpUnitBridgeIsolatedTest.php:   * @expectedDeprecation Drupal\deprecation_test\Deprecation\FixtureDeprecatedClass is deprecated.
core/tests/Drupal/Tests/Core/Test/PhpUnitBridgeTest.php:   * @expectedDeprecation Drupal\deprecation_test\Deprecation\FixtureDeprecatedClass is deprecated.

I wonder if we need a new @group that will let us test things like this without implying they are actually going to be removed.

This implements the tests using @expectedDeprecation ✌️

This looks great, just a couple of minor items

1. +++ b/core/.phpunit.result.cache
   @@ -0,0 +1 @@
   +C:37:"PHPUnit\Runner\DefaultTestResultCache":533:{a:2:{s:7:"defects";a:2:{s:84:"Drupal\KernelTests\Core\Asset\LibraryDiscoveryIntegrationTest::testDeprecatedLibrary";i:3;s:84:"Drupal\Tests\Core\Asset\LibraryDiscoveryCollectorTest::testLibrariesExtendDeprecated";i:4;}s:5:"times";a:3:{s:84:"Drupal\KernelTests\Core\Asset\LibraryDiscoveryIntegrationTest::testDeprecatedLibrary";d:2.714;s:84:"Drupal\Tests\Core\Asset\LibraryDiscoveryCollectorTest::testLibrariesExtendDeprecated";d:0.035;s:81:"Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testLibraryOverrideDeprecated";d:0.037;}}}
   

   this shouldn't be in the patch, might be worth adding to your .git/info/exclude

2. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -544,6 +551,98 @@ public function testLibraryWithLicenses() {
   +    $this->themeManager = $this->createMock('Drupal\Core\Theme\ThemeManagerInterface');
   +    $this->activeTheme = $this->getMockBuilder('Drupal\Core\Theme\ActiveTheme')
   ...
   +    $this->themeManager = $this->createMock('Drupal\Core\Theme\ThemeManagerInterface');
   +    $this->activeTheme = $this->getMockBuilder('Drupal\Core\Theme\ActiveTheme')
   

   Should we be using class constants for new code?

+++ b/core/modules/system/tests/modules/theme_test/theme_test.libraries.yml
@@ -5,3 +5,17 @@ theme_stylesheets_override_and_remove_test:
+  deprecated: 'The "%library_id%" asset library is deprecated in drupal:9.1.0 and is removed from drupal:9.0.0. Use another library instead. See https://www.example.com'
...
+  deprecated: 'The "%library_id%" asset library is deprecated in drupal:9.1.0 and is removed from drupal:9.0.0. Use another library instead. See https://www.example.com'

I know this is just test code but I realised the version numbers make no sense, we deprecate it after we remove it. In the other test case we say "is removed from drupal:10.0.0".

Out of scope for here but to avoid confusing test deprecations with real deprecations should we use fake version numbers like X.0.0 and Y.0.0 in tests?

+1 to use fake version numbers and updated the patch accordingly.

One usage missied

Hmm, I thought changing this might need a policy discussion as we haven't done it anywhere else yet. But as you agree, let's be bold and mark RTBC and see what core committers think. Otherwise the patch is good, this was just a last-minute nit.