Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code

Should we also be able to detect calls inside the running Drupal site?

We bubble errors up to the runner from the site under test.

See \Drupal\Core\Test\HttpClientMiddleware\TestHttpClientMiddleware.

Added to @Mile23's test, I think the first test method should be a kernel test, but that's for another day.

Yeah I was expecting that to fail, or at least be marked as risky. Either my local phpunit.xml must be different or something in BTB::setUp is creating an assert and bypassing the risky tests check.

It doesn't help that I now get this error for every functional test, not just PhpUnitBridgeTest, even without the patch from #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation

1) Drupal\FunctionalTests\Core\Test\PhpUnitBridgeTest::testSilencedError
GuzzleHttp\Exception\ConnectException: cURL error 7: Failed to connect to localhost port 80: Connection refused (see http://curl.haxx.se/libcurl/c/libcurl-errors.html)

Note to guzzle: MAMP is on port 8888, like it says in SIMPLETEST_URL.

Can't find an issue about it. Also want to file an issue for the testbot to randomize port numbers. :-)

User error. :-)

@mile23
I think opening its own issue is the right thing to do :) I bet we can figure out a solution for the problem.

Inside \PHPUnit_Util_ErrorHandler::handleError we stop in the first if condition:

    public static function handleError($errno, $errstr, $errfile, $errline)
    {
        if (!($errno & error_reporting())) {
            return false;
        }

... The question is, why is the phpunit error handler executed. Debugging resulted for me to have the following code executed inside

\PHPUnit_Framework_TestResult::run

:

        if ($this->convertErrorsToExceptions) {
            $oldErrorHandler = set_error_handler(
                array('PHPUnit_Util_ErrorHandler', 'handleError'),
                E_ALL | E_STRICT
            );

            if ($oldErrorHandler === null) {
                $errorHandlerSet = true;
            } else {
                restore_error_handler();
            }
        }

I tracked down the problem, I believe. In the child process \PHPUnit_Framework_TestResult::run is used to execute the test.

In there we have two problems:

1.         if ($this->convertErrorsToExceptions) {
               $oldErrorHandler = set_error_handler(
                   array('PHPUnit_Util_ErrorHandler', 'handleError'),
                   E_ALL | E_STRICT
               );
   
               if ($oldErrorHandler === null) {
                   $errorHandlerSet = true;
               } else {
                   restore_error_handler();
               }
           }
   

   is executed AFTER $this->startTest($test); and so overrides the symfony error handler

2. Let's assume we swap those two around, the next problem is that $this->listeners in there is empty.

Does anyone else has some further observations?

Hmm. Interesting results from the test in #2488860-56: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation

$ SIMPLETEST_BASE_URL=http://localhost:8888/ ./vendor/bin/phpunit -c core/ --stop-on-error --testsuite functional --filter PhpUnitBridgeTest
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing 
F

Time: 26.47 seconds, Memory: 28.00MB

There was 1 failure:

1) Drupal\FunctionalTests\Core\Test\PhpUnitBridgeTest::testSilencedError
Failed asserting that format description matches text.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  This is the deprecation message for deprecated_test_function().
+

/Users/paul/pj2/drupal/vendor/symfony/phpunit-bridge/SymfonyTestsListener.php:198

FAILURES!
Tests: 1, Assertions: 3, Failures: 1.

Remaining deprecation notices (1)

\Drupal\views\Tests\ViewTestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\views\Functional\ViewTestBase: 1x
    1x in FunctionalTestSuite::suite from Drupal\Tests\TestSuites

So here's the problem:

During test time, we don't allow autoloading of \Drupal\Tests\views\etc but we *do* allow autoloading of \Drupal\Views\Tests\etc. We have to do this because a) history, and b) we use phpunit-based tests to test Simpletest base classes and traits.

So using phpunit --filter means that all tests are loaded and parsed, which loads, for instance, Drupal\Tests\views\Functional\DefaultViewsTest and all of the other views functional tests. Because this test is alongside Drupal\Tests\views\Functional\ViewTestBase in the filesystem, it doesn't have a use statement (that's our coding standard). And even if it did, it still wouldn't autoload.

Since the base class ends in *Base.php, phpunit never finds it to include, so it's not going to be found when a class uses it. PHP is left saying this: 'ViewTestBase? Oh yeah, I know that one... it's Drupal\views\Tests\ViewTestBase!' Then we get the @trigger_error().

In an ideal world, we'd move all simpletest tests to another directory like MODULE_NAME/tests/simpletest_src/ or something and be happy. But we don't live in that world. :-)

We could also require use statements for all classes in tests, so there's no ambiguity. Er, never mind. Still wouldn't autoload correctly, and we'd end up with the simpletest base class in this case.

And we could make a special namespace that's always autoloaded, like \Drupal\Tests\module\SuiteName\Base\ and move all base classes into them.

This is all based on this change: #2863267: Convert web tests of views

As per IRC chat, I've updated the deprecation policy: https://www.drupal.org/core/deprecation#how-test-base-class

And here's an issue to untangle ViewTestBase: #2876145: ConfigTranslationViewListUiTest is a WTB test in the Functional namespace

Ok, so we didn't need to change deprecation policy, because it was just a bad test.

Re #10: The culprit is KernelTestBase::runTestInSeparateProcess. This signals \PHPUnit_Framework_TestCase to exec() another phpunit process. But the way it does it is interesting:

It takes a number of values and pokes them into a template file. The template is then the process that's run by the exec(), and it executes the test.

We also get the handy-dandy \PHPUnit_Framework_TestCase::prepareTemplate(), which is a hook pattern immediately recognizable to any Drupal dev. :-) This lets us put whatever values we want into the template, and also lets us change the template.

The template: https://github.com/sebastianbergmann/phpunit/blob/master/src/Util/PHP/Te...

I couldn't find where anyone had run into this problem in the symfony issues, so it might be an upstream fix.

Also another interesting problem here, with some patch overlap: #2878248: DrupalStandardsListener improper handling of @trigger_error() deprecation

Poking around on this issue led to this: #2880911: Remove unused KernelTestBase::getCompiledContainerBuilder()

A patch of tests to prove that deprecation errors aren't bubbling up from kernel or functional tests.

It turns out this problem applies to any test with process isolation, such as annotation with @runTestsInSeparateProcesses.

And the test to prove it. :-)

Calling this a bug because we really can't rely on deprecation for much unless this is fixed. I think it's fair to say the solution will lie upstream.

This is a patch that solves the problem of being able to test for deprecation errors in process-isolated tests.

It does not solve the problem of having deprecation errors in process-isolated tests fail tests that use deprecated code.

The reason @expectedDeprecation annotation didn't work is as follows:

Process-isolated tests end up sending a duplicate set of events to listeners. The first is the non-isolated one, and the second is isolated in a separate process.

The 'outside' non-isolated tests end up setting an expectation that fails, because the test never ends up being run (from the perspective of this 'outside' listener).

We solve this by subclassing SymfonyTestsListener and forcing it to not pay attention to tests which will eventually be run in isolation. Then we modify phpunit.xml.dist to use our new listener, and we also add a new isolated test template which adds this listener to the isolated test run.

So yay, we can at least test for deprecation errors. See, for instance, the results of Drupal\KernelTests\Core\Test\PhpUnitBridgeTest::testDeprecatedClass. :-)

The reason deprecation errors don't fail tests is as follows:

We silence the errors with @trigger_error().

Symfony\Bridge\PhpUnit\DeprecationErrorHandler is supposed to surface those errors under certain circumstances.

DeprecationErrorHandler does this by 1) Gathering them up in the error handler object. 2) Telling you about them in a shutdown function. 3) Calling exit(1) if there were errors.

That's all fine and dandy, except that as part of process isolation, PHPUnit ignores the result code from the executed process. So the deprecation error handler does its job, but PHPUnit pretends it didn't.

I couldn't find any upstream issues about either of these problems, so here we have a patch and a comment.

Reroll after #2862885: Batch: Convert system functional tests to phpunit

Amazing work! I don't have the mental power to review this patch, but heads up for actually trying to solve it!

Aaaaaand the testbot linter doesn't like files named *.tpl.php. Imagine that.

Testbot issue: #2882278: PHP linter tries to lint template files.

Well you can apply the patch and see the results with this command:

SIMPLETEST_BASE_URL=[your url here] SIMPLETEST_DB=[your db here] ./vendor/bin/phpunit -c core/ --group Test

Note that if you change any of the expected deprecation error messages in any of the isolated tests, you'll get a fail. But if you take off the @group legacy and the @expectedDeprecation tags, you'll get a pass even though it should fail due to E_USER_DEPRECATION error.

Currently we fail in two places:

There were 2 failures:

1) Drupal\KernelTests\Core\Test\PhpUnitBridgeTest::testDeprecatedClass
Failed asserting that format description matches text.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  Drupal\deprecation_test\FixtureDeprecatedClass is deprecated.
+  Install profile will be a mandatory parameter in Drupal 9.0.
   Drupal\deprecation_test\Deprecation\FixtureDeprecatedClass is deprecated.
+

/Users/paul/pj2/drupal/vendor/symfony/phpunit-bridge/SymfonyTestsListener.php:198
/Users/paul/pj2/drupal/core/tests/Drupal/Tests/Listeners/DrupalPhpUnitBridgeListener.php:34

2) Drupal\FunctionalTests\Core\Test\PhpUnitBridgeTest::testErrorOnSiteUnderTest
Failed asserting that format description matches text.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  This is the deprecation message for deprecation_test_function().
+

/Users/paul/pj2/drupal/vendor/symfony/phpunit-bridge/SymfonyTestsListener.php:198
/Users/paul/pj2/drupal/core/tests/Drupal/Tests/Listeners/DrupalPhpUnitBridgeListener.php:34

#1 shows two deprecations, one as a result of #2156401: Write install_profile value to configuration and only to settings.php if it is writeable which has a less-than-helpful deprecation message.

#2 shows that page controllers absorb some error messages we need to address.

I give you: The Drupal fork of symfony/phpunit-bridge. :-)

I'll refer to this patch in an upstream issue.

The test run here will fail because the testbot still doesn't know how to lint a template file that starts with <?php.

This patch gives us the ability to fail tests with @trigger_error() in process isolation. It does this by 'hacking core' of symfony/phpunit-bridge, so we'll work on an upstream change.

If you apply the patch and run it, you still get a fail from Drupal\FunctionalTests\Core\Test\PhpUnitBridgeTest, which signals that somewhere within the functional tests, we're swallowing E_USER_DEPRECATED. That's the next thing to solve.

This patch comments out the @trigger_error() deprecation from ExtensionInstallStorage::__construct() because it fails every test that builds a container. Bad core, bad!

The best way to demo this is the same as the last patch:

SIMPLETEST_BASE_URL=[your url here] SIMPLETEST_DB=[your db here] ./vendor/bin/phpunit -c core/ --group Test

Then edit Drupal\KernelTests\Core\Test\PhpUnitBridgeTest to remove the @group legacy line and also the @expectedDeprecation lines. Then re-run the same test command from above. This shows you a process-isolated deprecation fail.

Upstream issue: https://github.com/symfony/symfony/issues/23003

Drupal-scope part-way solution: #2884530: Mark process-isolated test with @expectedDeprecation as risky

I made a 'blog post about this problem: http://mile23.com/content/how-phpunit-and-phpunit-bridge-make-false-pass...

Some incremental forward motion at https://github.com/symfony/symfony/issues/23003

Unfortunately it's a fix in 3.3, which we can't use #2882826: Constrain symfony/phpunit-bridge to 3.2.*

phpunit-bridge now has does the process-isolated dance: https://github.com/symfony/symfony/pull/24548/commits/ff379efb59de25bcdb...

Still a WIP.

This patch modifies #19 to update symfony/phpunit-bridge to dev-master, which we really don't want. :-) We should change to >3.3.10 eventually.

The only fail from this patch is Drupal\FunctionalTests\Core\Test\PhpUnitBridgeTest::testErrorOnSiteUnderTest() which tests whether a deprecation surfaces from drupalGet(Url::fromRoute('deprecation_test.route')); (it doesn't).

We might consider putting this into 8.4.x when it's all done.

So yah, basically the remainder of this issue is finding ways to test whether our error handler(s) swallows deprecation errors.

Let's see how many deprecation errors are hidden... :-)

Yup. Pretty terrifying.

So we have to deal with deprecations in symfony and twig and probably others, as well as Drupal core before we can add this.

It'd be great to have a way to filter them, so that we could say: Only show the deprecations in twig or whatever.

Or perhaps a way to just turn on the handler in code, so that you could submit a patch that performs changes due to deprecation, and also turns on the deprecation reporting. Then when we're done, we can remove the code that does the switch for permanent fail due to deprecation.

+++ b/core/composer.json
@@ -18,7 +18,7 @@
-        "twig/twig": "^1.35.0",
+        "twig/twig": "^1.23.1",

This is an unwanted change.

Reeeally curious to see what is actually failing other than the 10 bajillion things about ArgumentsResolver.
Also fixed the stray Twig change called out in #50, but that didn't make the interdiff.

Re: #50

Sorry about that. Must have been a branch mixup.

Re-add composer.lock changes and moved some fixes from #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1.

Here is another quick win.

\Symfony\Component\Routing\RouteCompiler::compilePattern throws LogicException if non utf8 path sets the utf8 to TRUE.

@alexpott ok, but I think if we want BC compatible solution then we should create sub-issues all these changes are out of the scope for this issue imo.

Fixes for

exception: [User deprecated function] Line 48 of core/modules/editor/src/Plugin/EditorBase.php:
The Drupal\editor\Plugin\EditorBase::settingsFormValidate method is deprecated since version 8.3.x and will be removed in 9.0.0.

and

exception: [User deprecated function] Line 57 of core/modules/editor/src/Plugin/EditorBase.php:
The Drupal\editor\Plugin\EditorBase::settingsFormSubmit method is deprecated since version 8.3.x and will be removed in 9.0.0.

@jibran then maybe we should detect UTF-8 and set it automatically.

How about this?

If you scroll down to the bottom of the last set of results, you'll see deprecation errors in Simpletest.Drupal\simpletest\Tests\SimpleTestTest. They occur because the deprecated traits are loaded during discovery, and trigger deprecation errors after their namespace.

These deprecations occur because SimpleTestTest uses TestDiscovery to find and run tests.

For instance, in SimpleTestTest::testWebTestRunner() we load the test runner form, via $this->drupalGet('admin/config/development/testing'). This in turn calls TestDiscovery::getTestClasses().

TestDiscovery::getTestClasses() uses reflection to gather information about the files it finds, including traits because the traits live within the namespaces of the other test files.

Since it's using reflection, the deprecated trait files are require'd, so their deprecation errors are triggered, and you see errors like this:

exception: [User deprecated function] Line 5 of core/modules/taxonomy/src/Tests/TaxonomyTranslationTestTrait.php:
Drupal\taxonomy\Tests\TaxonomyTranslationTestTrait is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\taxonomy\Functional\TaxonomyTranslationTestTrait

Even if you replace all usages of TaxonomyTranslationTestTrait with the non-deprecated one, you'll still see this exception, because it occurs during discovery.

If we were to limit discovery to *Test.php files, then we would never include the traits in our classmap, so they would never be autoloadable. Because that's how TestDiscovery works. :-)

The reason why the rest tests fails is the following:

In HEAD when bc_primitives_as_strings is enabled, we basically return the data as it is set on the entity object. In the normal cases its coming from the database, and as such its returned as string.
This patch enforces that boolean objects always contain integers, no matter what.
If you are 100% honest this problem is not specific to boolean items though. If you are honest any value stored in any field might be used with the AllowedValue field and as such potentially problematic.

At least for me it feels like having a way to disable some deprecation notices instead of all or nothing, might be a good idea.

This should fix utf8 warnings.

Generally agree that WTB shouldn't be how you discover deprecations. But we do have #2904566: Allow TestBase to optionally notify/fail @trigger_error() deprecations, amend run-tests.sh with a patch. :-)

We really do need to explore one other thing... The PR that fixes phpunit-bridge also modifies symfony/browser-kit, which we use via mink for functional[javascript] tests.

I couldn't figure out how to specify browser-kit ^3.3.10@dev (I didn't try very hard, but dev-master is 4.* and requires PHP 7), but we'll eventually pull it in anyway. So we should incorporate whatever changes are there by explicitly requiring it.

https://github.com/symfony/symfony/pull/24548

+++ b/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php
@@ -11,6 +11,12 @@
+  protected $skippedDeprecations = [
+    'As of 3.1 an Symfony\Component\HttpKernel\Controller\ArgumentResolverInterface is used to resolve arguments. In 4.0 the $argumentResolver becomes the Symfony\Component\HttpKernel\Controller\ArgumentResolver if no other is provided instead of using the $resolver argument.',
+    'Symfony\Component\HttpKernel\Controller\ControllerResolver::getArguments is deprecated as of 3.1 and will be removed in 4.0. Implement the Symfony\Component\HttpKernel\Controller\ArgumentResolverInterface and inject it in the HttpKernel instead.',
+    'The Twig_Node::getLine method is deprecated since version 1.27 and will be removed in 2.0. Use getTemplateLine() instead.',
+  ];
+

@@ -41,7 +47,17 @@ public function __invoke() {
+                      if (!in_array($message, $this->skippedDeprecations)) {

Could we work on a filter where we search the backtrace for a given namespace? We could have an env variable like PHPUNIT_BRIDGE_EXCLUDE=Symfony\Component\HttpKernel\

That would help us know when to swallow deprecation errors for WebTestBase as well.

This would be the inverse of setting SYMFONY_DEPRECATIONS_HELPER to a regex.

Note: We have an issue for the http kernel already on #2912169: Inject the argument resolver into HttpKernel::__construct

Also what the patches show is that WebTestBase tests are the ones exercising the majority of the deprecated code

See #72. A lot of those deprecation errors are triggered during WTB tests which involve discovery.

Maybe run-tests.sh and/or the simpletest UI runner need to say SYMFONY_DEPRECATIONS_HELPER=disabled inside the child processes of WTB/TB tests.

Do we have an issue to track all the deprecation usages of Drupal itself?

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -690,9 +691,20 @@ protected function curlHeaderCallback($curlHandler, $header) {
   +      // Swallow deprecation notices. WebTestBase will not be updated to handle
   +      // them.
   

   This sounds like we will swallow all of the deprecations, which is not the case. Can we update this line here?

2. +++ b/core/tests/Drupal/FunctionalTests/Core/Test/PhpUnitBridgeTest.php
   @@ -0,0 +1,32 @@
   +class PhpUnitBridgeTest extends BrowserTestBase {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Test/PhpUnitBridgeTest.php
   @@ -0,0 +1,33 @@
   +class PhpUnitBridgeTest extends KernelTestBase {
   

   👏 Yeah tests for both kernel and browser tests!

3. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListener.php
   @@ -0,0 +1,115 @@
   + * @internal
   + *   This class will be removed once all the deprecation notices have been
   ...
   + */
   

   Don't we want to have the general feature available for longer?

4. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListener.php
   @@ -0,0 +1,115 @@
   +    // Need to edit the file of deprecations.
   +    if ($file = getenv('SYMFONY_DEPRECATIONS_SERIALIZE')) {
   +      $deprecations = file_get_contents($file);
   +      $deprecations = $deprecations ? unserialize($deprecations) : [];
   

   Will parallel file operations be a potential source of future problems? It turns out its not, $this->runsInSeparateProcess = tempnam(sys_get_temp_dir(), 'deprec');. Thank you @alexpott

5. +++ b/core/tests/bootstrap.php
   @@ -166,3 +167,8 @@ function drupal_phpunit_populate_class_loader() {
   +if ($ser = getenv('SYMFONY_DEPRECATIONS_SERIALIZE')) {
   +  DeprecationErrorHandler::collectDeprecations($ser);
   +}
   

   Let's use $file as well here.

I don't think so. If your test is failing because of deprecation notices - mark the test @legacy and put an @expectedDeprecation annotation on it - or just fix it. We're in a special situation here because there are so many notices triggered by a test run that we have to handle it specially - rather than just fixing them. For me \Drupal\Tests\Listeners\DeprecationListener is just a way to handle the massive scope.

Fair. We should clearly document for contrib that in case they trigger errors, they need to add @legacy now.

Regarding a change notice, should we updated the existing one: https://www.drupal.org/node/2811561 or rather add a new one?

I'd be surprised if the deprecation practice of @trigger_error was widespread in contrib

Here is search results for Drupal 8 modules hosted on drupal.org. So far only two modules address and salesforce have @trigger_error statement in its own code.

1. address/src/Plugin/migrate/cckfield/AddressField.php
2. loft_data_grids/vendor/symfony/yaml/Inline.php
3. loft_data_grids/vendor/symfony/yaml/Unescaper.php
4. loft_data_grids/vendor/symfony/yaml/Yaml.php
5. loft_data_grids/vendor/symfony/yaml/Parser.php
6. composer/vendor/composer/composer/tests/Composer/Test/Util/ErrorHandlerTest.php
7. composer/vendor/composer/composer/tests/Composer/Test/Util/SilencerTest.php
8. composer/vendor/symfony/process/Tests/ProcessTest.php
9. potx/vendor/Twig/Filter.php
10. potx/vendor/Twig/TokenParser/AutoEscape.php
11. potx/vendor/Twig/Environment.php
12. potx/vendor/Twig/TokenParserBroker.php
13. potx/vendor/Twig/Node.php
14. potx/vendor/Twig/Template.php
15. potx/vendor/Twig/Filter/Method.php
16. potx/vendor/Twig/Test/Node.php
17. potx/vendor/Twig/Filter/Function.php
18. potx/vendor/Twig/Test/Function.php
19. potx/vendor/Twig/Test/Method.php
20. potx/vendor/Twig/Filter/Node.php
21. potx/vendor/Twig/Function.php
22. potx/vendor/Twig/Function/Node.php
23. potx/vendor/Twig/Function/Method.php
24. potx/vendor/Twig/Function/Function.php
25. potx/vendor/Twig/Node/Expression/Name.php
26. potx/vendor/Twig/ExpressionParser.php
27. potx/vendor/Twig/Loader/String.php
28. potx/vendor/Twig/Lexer.php
29. potx/vendor/Twig/Test.php
30. potx/vendor/Twig/Extension/Escaper.php
31. potx/vendor/Twig/Extension/Core.php
32. potx/vendor/Twig/Autoloader.php
33. salesforce/modules/salesforce_mapping/src/SalesforcePullEntityValueEvent.php
34. salesforce/modules/salesforce_mapping/src/SalesforceQueryEvent.php
35. salesforce/modules/salesforce_mapping/src/SalesforcePushParamsEvent.php
36. salesforce/modules/salesforce_mapping/src/SalesforcePushEvent.php
37. salesforce/modules/salesforce_mapping/src/SalesforcePushOpEvent.php
38. salesforce/modules/salesforce_mapping/src/SalesforcePullEvent.php

If we have to be sure that we won't introduce a single fail for contrib then we need to implement a kill switch and have DrupalCI enable it for contrib testing and disable it for core. And then we'll need to provide some sort of UI on drupal.org for projects to enable deprecation testing. I think what might be best is to commit this to 8.5.x as early as possible and ensure that the CR has really good instructions for contrib so that when they do a run their project's tests against 8.5.x they know what to do.

Don't they run it against the latest 8.5.x first? It feels like having some form of announcement could prevent issues in the first place.

@Chi
Thank you for your grep! This is a nice list as all those exceptions are in vendor code / non runtime code like migrations. Given that the risk seems to be quite low in the first place

There is still almost 6 months before the 8.5 release, so I have the hope that if we publish a change record now, a lot of people will see it.
On the other hand I doubt there will be many people using something like that on custom modules, even it would be nice :)

+++ b/core/scripts/run-tests.sh
@@ -305,6 +305,10 @@ function simpletest_script_help() {
+  --suppress-deprecations
+
+              Stops tests from failing if deprecation errors are triggered.

🙃 The other parameters are documented in the same line

I was worried quite a bit about custom/contrib code suddenly failing, but I guess @trigger_error isn't that used actually.

Ah gotcha, no worries

Look at all that technical debt being uncovered - this is pretty cool.

🎉👌❤️

How did I not find this issue until now!?

This may be my favorite patch of all of 2017, definitely in the top 10. (The patch looks great BTW.) This will single-handedly make the entire Drupal ecosystem more aware of deprecations, and therefor help the entire ecosystem to gradually update.

I had an early taste of how well that works in #2884337: Fix Symfony 3.2 deprecation notices, because it was a pure unit test that was triggering a Symfony deprecation notice, which caused DrupalCI to send me an e-mail about the failed test! Resulted in a very easy release: https://www.drupal.org/project/big_pipe_sessionless/releases/8.x-1.1. This sort of "fix the fail + tag release" is fun: it's trivial to do, encourages a new release, and allows you to gradually keep with the times. IOW: an important step towards https://dri.es/making-drupal-upgrades-easy-forever
Exciting! 🙃

+++ b/core/composer.json
@@ -47,7 +47,7 @@
+        "symfony/phpunit-bridge": "^3.4.0-BETA2"

How about "symfony/phpunit-bridge": "~3.4.0@beta"? That way 'prefer-stable' will kick in once they release.

Other than that, I used PhpUnitBridgeIsolatedTest to figure out that isolated tests get their deprecation medicine properly by modifying the annotations. Also using the same technique I found that --suppress-deprecations turns the deprecation fails off properly.

So yay. :-)

Thanks to @alexpott for the excellent work on this.

Updated the change record to talk about how to turn it off and on from the command line, and included the new listener: https://www.drupal.org/node/2811561

Other than the version spec change, it's RTBC.

Superduper. :-)

It looks like MigrationDirectoryTest isn't testing for the deprecation, so it needs a follow-up to un-@legacy.

OK, so the change to the test still performs the migration even though it's the deprecated directory. I was mis-remembering phpunit tests short-circuiting on fail, but the deprecation tests don't work that way.

All the other test modules have the new directory after the change in #2917883: Rename 'migration_templates' directories in core modules to 'migrations' so we're still demoing that.

So it's still an out of scope change but it fixes the test and it allows the change here to pass. I guess we don't need a followup.

Well it was out of scope in that the test was broken, but sort of in scope in that the test was broken because we don't have this patch in yet. :-)

Now it looks like we get to do the same thing with #2920001: Add cacheable HTTP exceptions: Symfony HTTP exceptions + Drupal cacheability metadata

Woot. :-)

It’s great to see this patch working so well!

It will also help me keep #2893804: Remove rest.module BC layers up-to-date!

OK this looks great and the fact it keeps breaking shows how much we need it.

Committed 75dcf9a and pushed to 8.5.x. Thanks!

There's a tracking issue for removing all deprecated code usages here #2607260: [meta] Core deprecation message introduction, contrib testing etc., just slightly updated it now this has landed.

Thank you! I almost think this should be a release highlight given how important this is for the longtime upgrade strategy of Drupal.

Agreed. Thanks for all the awesome work here, folks!

This is great, but I think it might also be a problem for contrib.

Once a message is removed from being excluded, it will fail on contrib. But unlike core, contrib can not just use the new API if it's not yet in a stable release. For example, what we are seeing here are I think such messages: https://www.drupal.org/node/2919408#comment-12335861, not even sure if they are even caused directly by us.

One solution might be that contrib uses only the supported or maybe pre-release core version, I think one of those two should be the default, not Development.

One solution might be that contrib uses only the supported or maybe pre-release core version, I think one of those two should be the default, not Development.

Mh, on the other hand I think running tests for the next version also adds a lot of value in terms of catching bugs upfront.
I am wondering whether core should maybe not remove these checks for one minor version even it is fully compatible.

Well, but it's sometimes really hard to keep code and especially tests passing on multiple minor versions, so at least going for Pre-release would imho be preferable, that still gives you some warning a while before the next release. and you could still run a test development manually or on a schedule. But it's really annoying if all your patch testing is suddenly broken because core added some fancy new thing that ou can't yet use anyway :)

But yes, as a rule, only adding a trigger_error()/removing it from the exclusion list in the next minor might be a good idea.

Just a note discussion from #138 to #140 now going on in #2607260: [meta] Core deprecation message introduction, contrib testing etc..

FYI: I opened #2922451: [policy no patch] Make it possible to mark plugins as deprecated to discuss how we should mark plugins as deprecated

@Berdir: you can set up automated testing of both the current and development branch. The development branch will be failing more often indeed, because the non-deprecated successor to the deprecated API may indeed not yet be available on the current branch.

I think a follow-up to allow contrib modules to temporary skip certain deprecations would be very valuable indeed.

This was a blocker for #2866779: Add a way to trigger_error() for deprecated hooks

Updating metadata for #144.

Based on #2874909-79: Update Symfony components to 3.3.* we run into this problem: #2922887: Drupal DeprecationListener only works for process-isolated tests

From #138

Once a message is removed from being excluded, it will fail on contrib. But unlike core, contrib can not just use the new API if it's not yet in a stable release.

From #143

I think a follow-up to allow contrib modules to temporary skip certain deprecations would be very valuable indeed.

I have hit this exact problem in Rules module. #2883680: Force all route filters and route enhancers to be non-lazy was committed on 24th October, but from 10th November (after the commit in #134 above) the Rules tests fail at 8.5 because we were using the deprecated RouteEnhancerInterface. I can fix the code to use the new EnhancerInterface and the tests pass at 8.5 but now fail at 8.4 due Interface 'Drupal\Core\Routing\EnhancerInterface' not found

From #98 we had

We should clearly document for contrib that in case they trigger errors, they need to add @legacy now.

I would be interested to learn how we can make contrib tests pass at both 8.5 and 8.4 as we want to start using the correct new class.

@jonathan1055
One thing we could do is the following:

• Test contrib on the last minor instead by default
• Once we add a deprecation error, also add it to the list
• Remove it from the list at the beginning of the next minor cycle

Anyone has thoughts about it?

Thanks @dawehner and @alexpott. I am fine with contrib failing as soon as code is deprecated - my module runs daily tests at 8.3, 8.4 and 8.5 dev. I should have reported my problem on #2883680: Force all route filters and route enhancers to be non-lazy because that change has been committed to 8.5 but not 8.4. So now, if I leave the code unfixed it fails at 8.5 but if I fix it to run at 8.5 it fails at 8.3 and 8.4. That issue is now closed unfortunately because the commit was done a couple of weeks before deprecated code triggered a test failure.

This is a disaster - most of my hundreds of tests now fail and there's nothing I can do about it because my tests DON'T use deprecated functions and my code DOESN'T use deprecated functions - I can't just mark @group legacy because the problem isn't in my code.

My module just has a DEPENDENCY on Rules - none of my tests actually use Rules or call Rules methods or services or anything. If I remove the dependency, all the tests run fine. If I add the dependency (and nothing else), most of my tests fail.

I don't see how this change provides a gradual and smooth path toward removing @deprecated code. The effect I see is that every module needs to remove this code immediately or all the tests break, and since you can break the tests of your dependant modules, basically an entire ecosystem of modules can be thrown into immediate failure.

I also share the concern of #147 - the change, at least in the case of RouteEnhancer, is not backwards compatible, so everyone using my module will immediately be forced to upgrade their core to get a fix for this.

This amounts to a major API change and a BC break. I don't see how this constitutes an allowed change for a core minor point release.

https://www.drupal.org/core/d8-allowed-changes

Minor releases provide new improvements and functionality without breaking backward compatibility (BC) for public APIs.

I can't just mark @group legacy because the problem isn't in my code.

Actually you can, if they're not WTB tests. I know that's not a great solution.

All the *new* tests I add are PHPUnit tests, but I still have ~10,000 lines of tests with WebTestBase, developed over many years, that run just fine and perform an important role in making sure my modules run correctly. Converting them is a low priority because WebTestBase is SUPPOSED to be supported throughout D8. I don't need to have 40 hours of extra (unpaid volunteer) work dumped on me because converting to BrowserTestBase now seems to be a mandatory and immediate requirement in the middle of a minor point release.

Can anyone take a look at my patch in #112 on #2883680-112: Force all route filters and route enhancers to be non-lazy - It adds the new replacement classes to core 8.4 and 8.3 and therefore allows contrib to remove the deprecated code usage. That issue is closed and there has been no respone to it yet, but given that this problem is having a knock-on effect with modules such as Scheduler and SMS Framework, amongst many others, I would like to know if my solution works, and could be committed to 8.4 and 8.3

Jonathan

@alexpott OK, I get your point about the closed issue, but my patch is hardly "adding new stuff" it is simply creating a class of the same name as the new 8.5 classes, just using the already existing Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface, so no new active code. It seems like a very simple solution to solve this particular problem.

This should also be mentioned in the release notes for the alpha and the minor, since it disrupts test suites/CI integrations/etc. So reverting @webchick's tag revert, or at least adding it to both lists. :)

Just for reference, in relation to #150 and #156 above I have raised #2924899: Backport \Routing\EnhancerInterface to core 8.4 to discuss the backport of the replacement classes following the deprecations in #2883680: Force all route filters and route enhancers to be non-lazy

We have a follow-up here, to possibly backport some behavior from run-tests.sh --suppress-deprecations to 8.4.x: #2927636: Backport --supress-deprecations to run-tests.sh 8.4.x

Another follow-up: #2928645: \Drupal\Tests\migrate\Kernel\Plugin\MigrationDirectoryTest uses @expectedDeprecationMessage.