Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation

Should we remove this once we'll upgrade to symofony 2.7.0?

1. +++ b/core/composer.json
   @@ -31,7 +33,8 @@
   +    "symfony/phpunit-bridge": "^3.0@dev"
   

   Can we move this up with rest of the symfony components?

2. +++ b/core/phpunit.xml.dist
   @@ -10,6 +10,7 @@
   +    <ini name="error_reporting" value="-1" />
   

   I think this is not intentional.

This patch is adding a new library to drupal core. So there is a Dries approval necessary.

The issue summary need an update to better explain why this library is necessary.

+++ b/core/composer.json
@@ -31,7 +33,8 @@
+    "symfony/phpunit-bridge": "^3.0@dev"

Why do you want the dev-master and not regular version 2.7 or 2.8?

+++ b/core/phpunit.xml.dist
@@ -10,6 +10,7 @@
+    <ini name="error_reporting" value="-1" />

You say that this is intentional. The value is now set to 32767. Can you explain why you want it changed.

@dawehner: Do you have a link for me to that info. :-)

@dawehner: Thanks.

Working on reroll

Setting the status to "Needs review". Now the testbot will test the patch.

Here's a test and added the @group legacy to all the places in core where necessary. This change is going to need a change record and probably a core group announcement :)

The reason I went for the dev version is the ErrorAssert class that is coming in Symfony 3.2 - it just makes writing tests so much easier.

Good idea - we can wrap all of that up in a data provider to make testing anything additional simple.

Given we're moving away from Simpletest I think at best the integration with that should be a follow-up.

@dawehner nice change record, we probably need a docs page as well. And obviously before all of this we need to firm up the policy on #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases

The policy issue has been adopted now, so we can continue work here. Yay!

New patch reflecting the fact Symfony 3.2 is out and re-rolled on top of 8.4.x.

The bridge has changed - they now use annotations... see https://symfony.com/doc/current/components/phpunit_bridge.html

Trying to add deprecation errors to deprecated hooks here: #2866779: Add a way to trigger_error() for deprecated hooks

But the path ahead is unclear because core doesn't really even know what to do with @trigger_error() at this point, so postponing on this issue.

Here's a reroll

The test results of #44: https://www.drupal.org/pift-ci-job/646796

They tell us there's deprecated code in core (edited):

assertNoPattern() is deprecated and scheduled for removal in Drupal 9.0.0. Use $this->assertSession()->responseNotMatches($pattern) instead. See https://www.drupal.org/node/2864262: 1x
    1x in AssertLegacyTraitTest::testAssertNoPattern from Drupal\Tests\Core\Assert

Twig loader "Drupal\Core\Template\Loader\StringLoader" should implement Twig_SourceContextLoaderInterface since version 1.27: 2x
    1x in TwigExtensionTest::testCreateAttribute from Drupal\Tests\Core\Template
    1x in TwigExtensionTest::testFormatDate from Drupal\Tests\Core\Template

Twig loader "Drupal\Core\Template\Loader\StringLoader" should implement Twig_SourceContextLoaderInterface since version 1.27: 7x
    7x in TwigSandboxTest::setUp from Drupal\Tests\Core\Template

CckFieldPluginBase is deprecated in Drupal 8.3.x and will be
be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Plugin\migrate\field\FieldPluginBase
instead: 1x
    1x in TextFieldTest::setUp from Drupal\Tests\text\Unit\Migrate

(There's also a fail of Drupal\simpletest\Tests\UiPhpUnitOutputTest, which is probably related to one of these other deprecation failures.)

So we need child issues to remove usages of those deprecated items.

Also, at some point, we're going to mark TestBase and WebTestBase as deprecated. At that point, we'll be unable to test those classes using phpunit-based tests unless we just don't add @trigger_error() to the deprecation.

It'll also be interesting to see what explodes when, for instance, \Drupal::entityManager() throws this error as it should. That's every entity factory method.

Plan for deprecating hooks: #2870058: [policy, no patch] Document how to deprecate hooks

Linked here because if a defined function is marked as @deprecated, then the IDE can show you that it's deprecated. If a hook is deprecated, we only have @trigger_error() at run time.

Re-titling to focus on deprecation. The solution being proposed here doesn't do anything to help with things like the legitimate failure in #2705925-35: ImageItem presave() fails when pointing to a non-existing file entity. Where the node migration is throwing a simple warning and getting converted into an Exception, which is killing the migration.

Let's mark the tests as legacy and then file followups to remove it in the case of the Twig and Migrate tests - at least we get the benefit of this sooner rather than later.

+++ b/core/modules/simpletest/src/Tests/UiPhpUnitOutputTest.php
@@ -35,9 +35,9 @@ public function testOutput() {
-    $this->assertEqual($output[18], 'HTML output was generated<br />');
+    $this->assertEqual($output[19], 'HTML output was generated<br />');
     // Check that URLs are printed as HTML links.
-    $this->assertIdentical(strpos($output[19], '<a href="http'), 0);
+    $this->assertIdentical(strpos($output[20], '<a href="http'), 0);

This change is because the bridge adds an additional line of output.

    public function startTestSuite(\PHPUnit_Framework_TestSuite $suite)
    {
        $suiteName = $suite->getName();

        if (-1 === $this->state) {
            echo "Testing $suiteName\n";

It's the echo here...

Missed one.

Created #2870134: Twig loader "Drupal\Core\Template\Loader\StringLoader" should implement Twig_SourceContextLoaderInterface since version 1.27 to deal with the Twig deprecation.

Created #2870138: \Drupal\migrate_drupal\Plugin\migrate\cckfield\CckFieldPluginBase is deprecated - replace usages for the migrate one.

It also occurs to me that we need a followup to implement something for BrowserTestBase to catch deprecated usages in the system under test.

+1 on @group legacy, with follow-ups.

Here's a test you can add to the patch to prove that @trigger_error() gets swallowed somewhere in a functional test, so we do need a follow-up for BrowserTestBase.

Also, the change record references Symfony\Bridge\PhpUnit\ErrorAssert which I couldn't find documented, and which my IDE couldn't find.

Opened follow up #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code and fixed CR. Re-uploading patch in #52 since the last patch on the issue should be the one to commit.

Kewl. Sorry about the extra patch.

@Mile23 np - it's useful! I linked it from the new issue.

The migrate issue is #2833206: Convert Migrate's cckfield plugins to use the new field plugins - there was one already.

Go go go. :-)

Note that symfony/phpunit-bridge also gives us some handy mocks: https://symfony.com/doc/current/components/phpunit_bridge.html

Yay for legacy testing and the @group legacy... also adding a deprecation notice to a test class is interesting.

Deprecation trigger comes from #2824610: Rename DedupeBase/DedupeEntity process plugins to MakeUnique and add documentation

Provisionally RTBCing, because I can run --testsuite unit faster than the testbot.

Update: Never post something like that when it only says 90% complete.

Remaining deprecation notices (2)

The Drupal\Tests\migrate\Unit\process\MigrationTest is deprecated in
Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use Drupal\Tests\migrate\Unit\process\MigrationLookupTest: 1x
    1x in UnitTestSuite::suite from Drupal\Tests\TestSuites

The Drupal\migrate\Plugin\migrate\process\Migration is deprecated in
Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use Drupal\migrate\Plugin\migrate\process\MigrationLookup: 1x
    1x in MigrationTest::testTransformWithStubSkipping from Drupal\Tests\migrate\Unit\process

Thanks @Mile23!

Updated deprecation policy to tell people not to deprecate tests: https://www.drupal.org/core/deprecation

Once this lands we should update the policy with a note about testing.

Still RTBC since #65 fixes all the things.

#2712647: Update Symfony components to ~3.2 is in so the patch needs a reroll.

+++ b/composer.lock
@@ -4316,6 +4316,68 @@
+            "version": "v3.2.7",

This can be updated to 3.2.8 now.

Rerolled because of #2712647: Update Symfony components to ~3.2. Also that added some more deprecated code paths so fixed them or added @group legacy as necessary.

Thanks back to RTBC.

Fixing those three YAML files, let's see how this one does.

I've reviewed the changes @catch has made in #75. The fun fact about the YamlTest is that it existing to detect where the duplicate keys are not the same. One of the supported parsers trusts the first and the other trusts the last. Which is good because removal is therefore for correct.

+++ b/core/modules/views/config/schema/views.display.schema.yml
@@ -49,9 +49,6 @@ views.display.page:
-        expanded:
-          type: boolean
-          label: 'Expanded'

This is a non root schema duplicate so order is relevant. The only config I found that has this key is core/modules/content_moderation/tests/modules/content_moderation_test_views/config/install/views.view.latest.yml and the order is the as after this patch so @catch has removed the correct one.

Added review credit for @daffie and @jibran.

Committed/pushed to 8.4.x, thanks!

@trigger_error() conflicts with DrupalStandardsListener: #2878248: DrupalStandardsListener improper handling of @trigger_error() deprecation