[Symfony 5] The "Symfony\Component\Debug\BufferingLogger" class is deprecated since Symfony 4.4, use "Symfony\Component\ErrorHandler\BufferingLogger" instead

Thanks for the patch.

1) Found reference to BufferingLogger in 9.0 dev code in these files:

• core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceSettingsTest.php
• core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
• vendor/symfony/debug/BufferingLogger.php
• vendor/symfony/debug/CHANGELOG.md
• vendor/symfony/debug/Debug.php
• vendor/symfony/debug/ErrorHandler.php
• vendor/symfony/error-handler/BufferingLogger.php
• vendor/symfony/error-handler/Debug.php
• vendor/symfony/error-handler/ErrorHandler.php

2) The patch swaps:

Symfony\Component\Debug\BufferingLogger

with

Symfony\Component\ErrorHandler\BufferingLogger

and removes the deprecation warnings from DeprecationListenerTrait.php.

Looking at vendor/symfony/debug/BufferingLogger.php it has:

@deprecated since Symfony 4.4, use Symfony\Component\ErrorHandler\BufferingLogger instead.

so this looks correct.

3) The patch did not apply cleanly to 9.0 so moving back to "Needs work"

[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3117857.patch 
patching file core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceSettingsTest.php
patching file core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
Hunk #1 FAILED at 115.
Hunk #2 succeeded at 160 (offset -9 lines).
1 out of 2 hunks FAILED -- saving rejects to file core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php.rej

Thanks for the reroll. The patch applied cleanly and not sure this can be manually tested, so marking RTBC.

[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3117857-5.patch
patching file core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceSettingsTest.php
patching file core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php

Needs a re-roll.

Rerolled patch.

@longwave I don't understand your comment. Are you referring to this:

+++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceSettingsTest.php
@@ -9,7 +9,7 @@
 use Drupal\Tests\field\Traits\EntityReferenceTestTrait;
-use Symfony\Component\Debug\BufferingLogger;
+use Symfony\Component\ErrorHandler\BufferingLogger;

?

BufferingLogger is used in core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceSettingsTest.php, here:

  public function register(ContainerBuilder $container) {
    parent::register($container);
    $container
      ->register($this->testLogServiceName, BufferingLogger::class)
      ->addTag('logger');
  }

Oh! Of course, that make sense. Thanks for spelling that out for me. I agree with you on "we should also be explicit about the code we rely on".

Marking needs work for the dependency changes.

Thanks for the update.

1) Reviewed the changes and they look fine.

2) Compared patches in #12 and #18 to confirm the composer changes were the only ones.

3) Re-checked the whole patch.

4) Applies cleanly:

[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3117857-18.patch 
patching file composer.json
patching file composer.lock
patching file composer/Metapackage/DevDependencies/composer.json
patching file core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceSettingsTest.php
patching file core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php

5) Searched for symfony/debug after applying patch and it's referenced in:

a) composer.lock - autogenerated

b) composer/Metapackage/CoreRecommended/composer.json - ok

c) core/lib/Drupal/Core/Composer/Composer.php - $packageToCleanup

Should symfony/error-handler be added here? I don't understand what this is used for. The list in there doesn't match up with composer.json.

6) Searched for symfony/error-handler after applying patch and it's referenced in:

a) composer.lock - autogenerated

b) composer.json - added via patch

7) Searched for BufferingLogger after applying patch and it's only referenced in the code handled by the patch.

8) I'm going to assume 5c isn't a thing and that there is no good way to test manually. Since tests pass, marking this RTBC.

@Kristen Pol, thanks for your great review!

Core is directly dependent upon prophecy, we should always declare dependents that we are using and not rely on the fact that they are pulled in transitively. Everywhere that we have use Prophecy\Argument; for example. If phpunit decides to replace prophecy with something else, or bumps the version of it to something incompatible with our usage, our code breaks.

-- @mixologic

Re #20.5c, As @mixologic said on slack on Mar 20, 2020, "we should always declare dependents that we are using and not rely on the fact that they are pulled in transitively", So it's good to add symfony/error-handler .

Queued the testing against 9.0.x, RTBC+1

Thanks @jungle. I'm a bit confused. You have RTBC+1 but you also have:

Re #20.5c, As @mixologic said on slack on Mar 20, 2020, "we should always declare dependents that we are using and not rely on the fact that they are pulled in transitively", So it's good to add symfony/error-handler .

Do you mean to add it to core/lib/Drupal/Core/Composer/Composer.php? As noted in #20.5c, it's missing from that. If it needs to be added, this issue should go back to "Needs work".

+++ b/composer.json
@@ -25,7 +25,7 @@
+        "symfony/error-handler": "^4.4",

I meant adding symfony/error-handler to composer.json from my understanding of what @mixologic said which is good,

c) core/lib/Drupal/Core/Composer/Composer.php - $packageToCleanup

Should symfony/error-handler be added here?

At first glance, I thought it's talking about adding to composer.json. The comment in #15 was in my mind . My bad. Apologize.

Back to the question. \Drupal\Core\Composer\Composer::$packageToCleanup is to make packages smaller by removing unnecessary tests/docs files etc.

+++ b/composer.json
@@ -25,7 +25,7 @@
-        "symfony/debug": "^4.4",
+        "symfony/error-handler": "^4.4",

symfony/debug was removed, symfony/error-handler was added, so yes, I think it's good to add ['symfony/error-handler'] => ['Tests'], to there.

Adding ['symfony/error-handler'] => ['Tests'], to \Drupal\Core\Composer\Composer::$packageToCleanup

To make it more clear

• \Drupal\Core\Composer\Composer::$packageToCleanup is from the file core/lib/Drupal/Core/Composer/Composer.php
• Tests is the directory where is its tests. See https://github.com/symfony/error-handler

Thanks for the update.

1) Confirmed the only change was to core/lib/Drupal/Core/Composer/Composer.php.

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -75,6 +75,7 @@ class Composer {
     'symfony/dom-crawler' => ['Tests'],
     'symfony/filesystem' => ['Tests'],
     'symfony/finder' => ['Tests'],
+    'symfony/error-handler' => ['Tests'],
     'symfony/event-dispatcher' => ['Tests'],

Nitpick: Not alphabetical, but it was already like that with filesystem and finder before event-dispatcher, so not marking "Needs work".

2) Patch applies cleanly:

[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3117857-25.patch
patching file composer.json
patching file composer.lock
patching file composer/Metapackage/DevDependencies/composer.json
patching file core/lib/Drupal/Core/Composer/Composer.php
patching file core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceSettingsTest.php
patching file core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php

3) Marking RTBC! Thanks!

Committed/pushed to 9.1.x and 9.0.x, thanks!