[meta] Core deprecation message introduction, contrib testing etc.

This issue is going to spawn plenty of work during the cycle.

Clarifying the title a little. I think this is a good idea.

This also raises the question of when we commit patches that remove deprecated code usage in order to meet this requirement. There are two competing concerns:

1. Removing deprecated code usages does risk introducing regressions and disrupting other patches so it seems inadvisable to allow it for patch releases for that reason.
2. On the other hand, committing such changes only to the next minor means that bugfixes become harder to backport.

A middle ground might be always targeting these sorts of changes for the RC phase for each minor (so 1 month out of every 6). We could also distinguish between 1:1 git diff --color-words replacements, and deprecations that require refactoring. The former are lower risk but more likely to introduce merge conflicts, so we cherry-pick those. The latter are more likely to be contained to a handful of specific places, but more likely to introduce regressions, so they could be targeted for the next minor during its dev phase.

Removing deprecated code usages does risk introducing regressions and disrupting other patches so it seems inadvisable to allow it for patch releases for that reason.

We need to ensure that those conversion issues just do the conversions and nothing else. Though in case we add regressions and find them,
we almost always learn something about Drupal in a given way this is itself of a really high value, so I'm kinda less afraid of breaking something than other people are.
Old code around adds costs like people slowing down in their reviews, which well, slowly adds up over hundreds of contributors.

On the other hand, committing such changes only to the next minor means that bugfixes become harder to backport.

Well, this is just true for the next version, given that we just support 8.0 or 8.1.

One general question: Do we allow to backport the deprecated usages? So will we backport these things to 8.0.x while working on 8.1.0?

#15, what are your thoughts on the best way to accomplish this?

I just ran an inspection in PhpStorm. We have about 500 things marked as @deprecated, and about 18,000 deprecated code usages in core.

We can can have a test patch/issue that searches for @deprecated and add trigger_errors() everywhere. That won't catch the constants, though.

We have a handful of @expectedDeprecation in tests right now (three? or so from a quick search).

Maybe start a per-module issue like we are doing for PhpUnit and list them all here? In this case, would it be (1) remove the deprecated usages of everything w/in that module or (2) replace all things that are deprecated in that module all across core?

The first would be better from a re-roll purgatory perspective, but the later would be better for consistency.

Copy-pasting my comment from #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code also into this issue, alexpott suggested to create an issue as part of/blocking this meta, to figure out when/how to exactly add non-suppressed deprecation messages to make it minimally painful for contrib:

berdir:
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.

dawehner:
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.

berdir:
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.

#19 seems like an elegant and good solution to me.

To answer #18, quoting what I wrote at #2870194-143: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code

@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.

Here's a first child issue: #2922487: Follow-up for #2910211: fix all deprecation warnings.

@jonathan1055 has pointed out in #2870194-147: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code that contrib is facing problems having tests pass both on 8.5.x and 8.4.x - it looks like we need to allow contrib to take their own approach to managing their tech debt and when they move to the un-deprecated code path. I think @catch's and @dawehner's suggestions about leaving the deprecations listed in the skipped for a while makes a kinda of sense but what that means is that we can't be sure that core is free from usages and importantly is not adding new usages. So I've opened up #2923267: Allow test classes to specify skipped deprecations too to help.

Another solution is adoption of #2667536: Perform high level static analysis on core code
So core & contrib can track usage of deprecated stuff

Letting contrib specify their own skipped deprecations seems like an OK addition, but I'd still do the staggering so they don't have to.

I'm not worried about the actual timings, doing the staggered approach is fine, but it is not an either/or solution. Contrib has to be able to skip deprecation messages when the solution for the deprecation is not yet available at lower major releases, even at dev level. To use the example I quoted from #24 above, in #2883680-111: Force all route filters and route enhancers to be non-lazy - the new class which replaces the deprecated code in 8.5 has not been committed to 8.4, so when I remove the usage of deprecated code and use the new class instead the tests then pass at 8.5 but fail at 8.4 and 8.3. Hence contrib cannot actually fix our code until the replacement is available in all supported major releases. The solution in #2923267: Allow test classes to specify skipped deprecations too is vital not just a nice-to-have, if contrib wants to keep a fully=green test suite at 8.3, 8.4 and 8.5

I think changing our backport policy due to the deprecation messages would be wrong. It makes sense to treat the next minor as the actual development branch and then backport bug fixes, as long its useful.

I totally agree that we should hide deprecations till the next minor, though I'm really not worried about us forgetting about those.

The question then is how does core remember to do things. For one, we should possibly group items in the skiplist by the release in which they were added, then it'll be clear which are from 8.4.x and which are from 8.0.x. Also having the follow-up issue every time will help.

Is there a list of tasks needed to be done before the first beta or so? To be honest its not obvious for me, when exactly this removing should be applied.

Can we do something like this?

diff --git a/core/tests/Drupal/Tests/Listeners/DeprecationListener.php b/core/tests/Drupal/Tests/Listeners/DeprecationListener.php
index 3673376..1a552ce 100644
--- a/core/tests/Drupal/Tests/Listeners/DeprecationListener.php
+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListener.php
@@ -36,7 +36,9 @@ public function endTest(\PHPUnit_Framework_Test $test, $time) {
    * A list of deprecations to ignore whilst fixes are put in place.
    *
    * @return string[]
-   *   A list of deprecations to ignore.
+   *   A list of deprecations to ignore. Keys are deprecation messages, values
+   *   are the first core version where the deprecation should become a failure
+   *   in tests.
    *
    * @internal
    */
@@ -109,8 +111,8 @@ public static function getSkippedDeprecations() {
       'CommentVariablePerCommentType is deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.x. Use \Drupal\node\Plugin\migrate\source\d6\NodeType instead.',
       'The Drupal\config_translation\Plugin\migrate\source\d6\I18nProfileField is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use Drupal\config_translation\Plugin\migrate\source\d6\ProfileFieldTranslation',
       'The Drupal\migrate_drupal\Plugin\migrate\source\d6\i18nVariable is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use Drupal\migrate_drupal\Plugin\migrate\source\d6\VariableTranslation',
-      'Implicit cacheability metadata bubbling (onto the global render context) in normalizers is deprecated since Drupal 8.5.0 and will be removed in Drupal 9.0.0. Use the "cacheability" serialization context instead, for explicit cacheability metadata bubbling. See https://www.drupal.org/node/2918937',
-      'Automatically creating the first item for computed fields is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\TypedData\ComputedItemListTrait instead.',
+      'Implicit cacheability metadata bubbling (onto the global render context) in normalizers is deprecated since Drupal 8.5.0 and will be removed in Drupal 9.0.0. Use the "cacheability" serialization context instead, for explicit cacheability metadata bubbling. See https://www.drupal.org/node/2918937' => '8.6.0',
+      'Automatically creating the first item for computed fields is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\TypedData\ComputedItemListTrait instead.' => '8.6.0',
     ];
   }
 

So I think only add a trigger_error() and/or remove a deprecation from the exclusion list when the API exists in a tagged release is a good rule of thumb

We should never add anything to the exclusion list, and only ever remove. The exclusion list helps us surface existing tech debt, but adding to it adds to the debt.

Our deprecation policy should be to mark stuff @deprecated and then in the next minor, add the @trigger_error().

That is, we shouldn't add @trigger_error() to anything but dev (not alpha or beta or RC or releases), and we should only do it for @deprecated added during the last release. New @deprecateds don't get a trigger until the next minor. Hopefully as we've deprecated things we've created issues to call @trigger_error(), tagged to the next dev branch.

Contrib can add @group legacy to tests, and they'd be wise to add an issue to their project about un-legacy-ing the test when the next minor dev is created. Then they can become aware of whatever real deprecation problems core has created for them. Ideally, they'd start using semantic versioning, like 8.x-5.x targeted to 8.5.x. Of course, we can't do that currently, because info files don't let you.

It seems like the problems contrib encounters should be Drupal's APIs, and not symfony versions or whatever, if we're managing our debt properly.

Sorry if this duplicates someone else's idea. If that's the case, then +1 for them. :-)

@Mile23 but the problem with:

Our deprecation policy should be to mark stuff @deprecated and then in the next minor, add the @trigger_error().

Is that Symfony adds deprecations too - so how do we cope with that?

#31/@catch: Yes.

Our deprecation policy should be to mark stuff @deprecated and then in the next minor, add the @trigger_error().

Even that seems problematically complicated. Just asking for us to miss adding triggers.

Re #33:

Symfony adds deprecations too - so how do we cope with that?

We only change symfony versions on minor releases, right? So core deals with that during dev, and then contrib deals with it during alpha, beta, etc, if it's even an issue. It seems like contrib should be doing core API rather than much symfony itself.

If it's any more arbitrary than that, we can't guarantee much anyway.

Re #35:

Just asking for us to miss adding triggers.

It's similar to how we do it now: For our own deprecations we're supposed to have an issue each for replacing the behavior, deprecating the old behavior (with @deprecated), and removing usages. So just add another step to that list.

Contrib can add @group legacy to tests, and they'd be wise to add an issue to their project about un-legacy-ing the test when the next minor dev is created.

@Mile23 I tried @group legacy over at #2922871: Fix test failures in D8.5 due to use of deprecated APIs and the number of test fails dropped from 36 to 14. 13 of the remaining 14 fails are in Web tests. It seems @group legacy doesn't work on (Simpletest) Web tests, only on (phpunit) Kernel and Browser tests. If so, what's the mechanism for web tests.

Convert to BrowserTestBase.

The recent change made in #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code 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 the deprecated 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 #27 - the change, at least in the case of RouteEnhancer, is not backwards compatible with core 8.4.x , so everyone using my module will immediately be forced to upgrade their core to 8.5.x 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.

Oh, and in regards to #38, the change record New PHPUnit based classes added for testing: BrowserTestBase and JavascriptTestBase says quite clearly:

Don't worry, Simpletest will need to be supported for Drupal 8, removing it would be an API break.

If this @deprecated change now requires contrib to immediately convert all the legacy WebTestBase tests to BrowserTestBase, that again falls way outside what is allowed in a minor point release of core.

From the change record for these deprecation fails: https://www.drupal.org/node/2811561

If you use run-tests.sh to run all your tests, you can now specify --suppress-deprecations on the command line, and this will ignore deprecation errors during test runs.

Using phpunit to run tests, you can specify SYMFONY_DEPRECATIONS_HELPER=disabled as an environmental variable. You can also specify SYMFONY_DEPRECATIONS_HELPER=strict to explicitly cause deprecations to fail. The best place to specify this is in a custom phpunit.xml.

So if you're rolling your own CI, then add these things.

In an ideal world, we'd have #2901677: Allow projects to contain their own drupalci.yml file and you could specify it in your project. But there are only so many hours in a day.

@TR the simplest "fix" for you right now is to change your module not to test against 8.5.x. If you are running the tests using drupal.org, on the automated testing tab of your project you can edit your test config to choose "Pre-release / supported" instead of "Development" this will mean your tests will only move to the latest branch when it is about to be released. As @Mile23 has pointed out if you are running the tests on your own infrastructure you have several other options. Calling something a disaster that allows you to know and plan how to address usage of deprecated code (even if it is in one of your dependencies) is a bit off. Managing such technical debt has been a problem in Drupal for years and we're on a road to improving this.

I think @Wim Leers' idea in #30 seems like a really good idea. It allows us to make it possible for contrib to have tests passing on the stable branch and the dev branch whilst an deprecation like #2883680: Force all route filters and route enhancers to be non-lazy is only in the dev branch.

I tried to validate whether this approach would be possible. One thing I realized by playing around with version numbers: We should compare against '8.6.0-dev', so we can start showing the deprecation message.

List of example comparisons:

version_compare('8.5.0', '8.6.0-dev')
-1
version_compare('8.5.0-dev', '8.6.0-dev')
-1
version_compare('8.6.0-dev', '8.6.0-dev')
0
version_compare('8.6.0', '8.6.0-dev')
1
version_compare('8.6.0-alpha1', '8.6.0-dev')
1

1. +++ b/core/tests/Drupal/FunctionalTests/Core/Test/PhpUnitBridgeTest.php
   @@ -22,6 +23,19 @@ public function testSilencedError() {
   +    $this->assertEquals('known_return_value', deprecated_test_always_skipped_function());
   ...
   +    $this->assertEquals('known_return_value', deprecated_test_never_skipped_function());
   

   Not sure these methods exist... plus this could be in a unit test - no need for this to be in a BrowserTestBase test. Perhaps a new unit test for DeprecationListener?

2. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListener.php
   @@ -33,84 +33,105 @@ public function endTest(\PHPUnit_Framework_Test $test, $time) {
   +      'This is a deprecation message which should be always skipped.' => '20.0.0-dev',
   

   Is twenty high enough ;)

3. One thought I had this morning was whether or not we're ready for the likely impact of this change. It feels very likely that when we open 8.6.x the branch will fail for deprecation errors. On one hand this will prevent 8.6.x development till the technical debt is paid by fixing it - or deferred by setting the deprecation to a later release. Both of which seem a very honest way of working with our tech debt but I wonder about the disruption to the issue queues. On the other hand this might not be that much of an issue because 8.6.x does not exist till we create it so perhaps the order is:
   1. Create 8.6.x
   2. See if the branch passes
   3. If it does not: fix each failing deprecation in a separate new 8.6.x issue where we can discuss whether fixing or deferring is the correct solution
   4. Then once, all tests green, do the issue move to update issues to 8.6.x

It feels very likely that when we open 8.6.x the branch will fail for deprecation errors

I think it would be less of an issue if we wouldn't have the technical debt, which we have right now. Couldn't we start opting into failing at the next version, when we have cleaned up the technical debt?

The summary had a couple paragraphs that were duplicated verbatim, so I've removed the duplicate.

I don't quite get #43.

I think we should just not backport un-suppressing messages, new deprecations, or new @trigger_error(), the same way we don't backport enabling coding standards rules. They are disruptive changes. We already have processes around announcing disruptive changes and informing the community about upcoming minor releases, announcements, emailing contributed module authors, etc. We should include this in those messages.

Contrib then has two options when there are deprecated usages:

1. Mark the test @legacy until they can fix it.
2. Test against the production branch only for their on-commit and issue testing default.

As I said to @catch immediately after the commit of #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code, I do think the change should have been announced on g.d.o/core before commit. We can still do this and we could include instructions for contrib developers in the announcement.

Are we sure all this deprecation hoop jumping is worth it. It seems like we're lumping more and more complexity onto the solution when all that seems to beg us to look back at the core of the problem and consider if there's something we need to simplify.

@neclimdul: Agreed. See #32. Do it as a policy rather than a bunch of code. We're already pretty good at the policy parts.

If contrib is seeing deprecations from symfony or any other core dependencies, then that's a bug in core akin to failing HEAD, not something contrib should silence. If contrib sees deprecation errors from core they can do @group legacy until they change their code.

The only code that's needed is to manage deprecation errors from WTB, since @group legacy doesn't turn that off.

But the problem is with changes like #2883680: Force all route filters and route enhancers to be non-lazy a module can't have green tests on 8.4.x and 8.5.x at the same time. One solution is that we should suppress the deprecation message till 8.6.x which is why @dawehner is exploring the solution above. I can't think of a policy that can fix this.

... a module can't have green tests on 8.4.x and 8.5.x at the same time.

That is true, but I have learned that this is OK - see posts #5-#9 on #2924899-5: Backport \Routing\EnhancerInterface to core 8.4

a module can't have green tests on 8.4.x and 8.5.x at the same time.

So there are three relatively simple solutions:

1) Don't target dev.

2) Have a next-minor dev branch, more like semantic versioning, that you assume will break due to core deprecations.

3) Turn off deprecation errors somehow. We should extend a @group legacy type behavior to WTB (because what's more legacy than WTB?), and also #2901677: Allow projects to contain their own drupalci.yml file to turn it off for DrupalCI. This solution is relatively simple for contrib, but not so much for core or drupalci_testbot. :-)

If we only commit @trigger_error() during dev, then this minimizes the amount of time this is a problem for contrib, which would be between alpha and release per minor. Or contrib could be adventurous and keep a branch that tries to track dev.

If I correctly understand the goals of #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code, we're trying to A. surface existing technical debt that exists in the form of usages of deprecated API's and B. Prevent further introductions of usages of those apis.

So the problem Im seeing here is that we're overusing *testing* as a way of accomplishing those goals. A deprecation usage is not a bug, and tests should not fail for merely using deprecated code. Tests should only pass or fail if the code does what its supposed to. If deprecation usage is a failure condition, then you're essentially forcing contrib maintainers to stay API compatible completely through the 8.x release cycle, which is problematic for a number of reasons, namely that it breaks the API contract in the first place and says "if you want your tests to pass, then you have to be compatible with the development branch and keep up with deprecations"

Using deprecated code is not a bug, it's merely an indicator of future incompatibility. That information should be surfaced, much like coding standards should be surfaced, but it should not be entagled with testing as if it were equivalent.

A normal test run should capture deprecation warnings, and provide those as an artifact of the build. And a build fail should only realistically be implemented if *new* deprecation usages are introduced in a patch or commit.

Mingling deprecation warnings with test results, and turning them into errors defeats the purpose of deprecating code in the first place, which is to give users of those API's fair warning that those API's need to change. If you break their tests because they are using a deprecated API, then you've removed testing as a tool they can use for project quality.

We introduced the ability for modules to pick which level of stability they want so as to allow the contrib developers more choice in the level of upstream change they wanted during their own development cycles. Deprecation notices should *inform* them, but not *force* them to do anything.

So now we get into a scenario where Contrib module A (8.x-1.0) is using a deprecated API from 8.4.x, e.g. RouteEnhancerInterface, In order to make tests pass, they make changes to their 1.x branch, to use the new Drupal\Core\Routing\EnhancerInterface. Now their tests will pass on 8.5.x, but not on 8.4.x. *then* somebody finds a security issue with Contrib module A, and the maintainer fixes it. Now the problem is if they release 8.x-1.1, that now has a hard requirement on drupal/core ^8.5, but that doesnt yet exist, so there isnt a way for existing site owners to upgrade to the 8.x-1.1 version of Contrib module A.

The alternative then is to keep their modules only testing against 8.4.x, and *hope* that there arent any security issues during the 2 month window of pre-release, during which time every contrib maintiner will have to upgrade all of their code to use new apis, which we've told them they have to do "sometime before 9.x" *not*, during every point release.

So this is why deprecation warnings should not be hitched to test results, and that tests should still pass regardless of whether or not a deprecated API is being used.

We need a proper mechanism to surface usages of a deprecation, especially new introductions of deprecated usages, but that should be the maintainers prerogative as to how to handle those deprecation warnings. Maybe they just need to quickly add a feature, and they don't, at that moment, have the time to take to learn the new api, and actually want to keep on using the old api's.

Im tempted to just add `--suppress-deprecations` to contrib tests on drupalci until we give them a way to choose what to do here.

Wow, that is massive. Thank you @Mixologic.
#56 Makes sense to me.

#56 +1000. Couldn't have put it better myself.

I agree this feels like something outside of testing which should cut down on a lot of complexity being added.

We can probably do some procedural things to help as well to help contrib targeting. Deprecation changes not being beta/rc approved? Earlier branching of next branch for deprecation targeting?

@neclimdul
Just do understand you correctly, you suggest that we basically scan contrib/custom code to see whether they call any deprecated APIs?
While static analysis is certainly really nice, how would we inform people that they do it? Would this require an additional step?

I think that calling deprecated code can be a bug. And so does PHP. There's a reason E_USER_DEPRECATED exists and also we have issues like #2885309: [PHP 7.2] each() function is deprecated. For core we have a policy that once something is deprecated we don't want to have an usages in regular run-time. If we do, that's a bug as per our policy.

However, as pointed out above this represents a problem for contrib because they are trying to work against the current branch and future branch. I think contrib needs to know both things - that their test pass regardless of deprecations and also which tests fail because of deprecations. And they should be able to implement their own deprecation policy too. So I think maybe a way out of this is to implement what @Mixologic suggests in #56 and add --suppress-deprecations to all contrib tests. This will give us time to discuss the best solutions going forward with less impact on contrib at the moment. For example, then we could add suppressing deprecations as a configurable option to the QA UI so contrib maintainers can add it a particular test run. That way they could configure a test run against the core dev branch with deprecation testing on and be informed about upcoming changes that affect their code.

I'm inclined to agree with #56, in that deprecated usages don't necessarily have to be fails, and we get to decide that. Also whether it's a fail or not is different for different scopes.

So if you're patching core, it should fail the tests, because we want to surface that debt early. If you're working on contrib, not so much, because in this regard Drupal is a framework with BC as folks have pointed out above.

This gives a +1 to suppressing deprecation fails on contrib with a follow-up to remove it before 8.5.0 release, in favor of whatever long-term solution we make before that happens.

It seems there's not an easy way to get a detailed report on deprecations from phpunit-bridge if you're not also failing tests. SYMFONY_DEPRECATIONS_HELPER=weak does give us a count of deprecation errors handled, just not any info about what they are or where they come from, and this info never shows up in the JUnit report. So for the use-case of run-tests.sh you'd have to look at the output from individual tests to learn that there were deprecations, like this:

Time: 4.09 minutes, Memory: 214.00MB

OK (1 test, 17 assertions)

Remaining deprecation notices (1639)

(FTR that's a run of Drupal\Tests\file_example\Functional\FileExampleTest after commenting out all the exceptions in DeprecationListener.)

There's also the SYMFONY_DEPRECATIONS_HELPER=weak_vendor option. This ignores all errors except those that come from a dependency in the vendor directory. Once core cleans up its use of deprecated symfony/twig stuff, this could be useful for contrib that has Composer-based dependencies.

Should tests against stable (8.4.x) use --suppress-deprecations and tests against development (8.5.x) not use --suppress-deprecations?

Wouldn't that achieve the best of both worlds?

@Wim Leers I think we should have a setting for whether deprecations should be suppressed or not to allow contrib authors to choose.

That makes even more sense. Allow --suppress-deprecations to be configured in each project's Automated testing configuration.

@Mixologic #56 is excellent and exactly what we contrib maintainers need. Separating out test failiures for code faults vs deprecation usage is absolutely correct.

@Mile32 in #62 it would be fine just to have the number of deprecation messages displayed, if it is not currenrtly possible to extract the full details. For any conciencious contrib maintainer just seeing the message and number of deprecations is enough to keep it on our radar, and prepare fixes to be committed when the supported core version allows us to. We have other ways to test and get the full details, e.g. locally and on Travis. Any maintainer who ignores the deprecation number it also likely to not bother fixing it even if the tests failed. Or probably would not even have tests for their module. So just showing an artifact (how ever minimal the information it conains) is enough for those who care, to alert them to the problem.

Making the tests fails for deprecation, and all this discussion around it, and the multiple modules it is now affecting, just takes effort away from doing the good stuff of maintainging contrib modules.

Added a testbot issue: #2926314: Contrib should run with --suppress-deprecations

Closed #2922537: "User deprecated function" should not cause a test to fail in the DrupalCI issue queue as a duplicate of this.

For the JSON API module, #2926413: RouteEnhancerInterface is deprecated in Drupal 8.5.0 is trying to work around the test failures due to the deprecation introduced by #2883680: Force all route filters and route enhancers to be non-lazy. It does so by adding @group legacy to the affected test classes and @expectedDeprecation to affected test methods.
It allows the module to pass tests on 8.5 then! But now it fails on 8.4 … because those expected deprecations only exist in 8.5.

So we can only go from green 8.4 tests and red 8.5 tests to green 8.5 tests and red 8.4 tests. It's impossible to make both green.

@group legacy tells the test system to ignore deprecation errors.

@expectedDeprecation tells the test system that it should fail the test if it doesn't see that specific deprecation error.

So if you're trying to make tests pass despite deprecation errors, you'd only use @group legacy.

How does --suppress-deprecations impact people that use phpunit directly either via phpstorm or on the CLI?

#71: that doesn't work for me locally. Am I running tests wrong?

@neclimdul if you use PHPUnit directly you need to add the listener yourself - so it is already up to you - if you don't deprecation testing doesn't happen - the errors are silenced after all. All deprecation testing in PHPStorm is not supported ATM because PHPStorm replaces the results printer and messes with the way PHPUnit tests is run already so the deprecation listener is not registered.

@Wim Leers I think that @expectedDeprecation works with @group legacy. All deprecations are treated as errors unless the test is in the legacy group. If the test is in the legacy group then to ignore a specific error I think you need to set @expectedDeprecation. It works this way is so that we can report on expected vs unexpected deprecations in legacy tests - which are the only types of tests where deprecations should fire.

As a contrib maintainer, this new "feature" is causing me some major problems - all of my modules suddenly stopped passing their tests because of this.

I agree with everything mentioned in #56. This breaks the promise of not breaking backwards compatibility until 9.0.

#74: No, I meant that only doing @group legacy still causes tests to fail locally. But they do pass on DrupalCI. Which is why I think I'm running tests wrong. And this probably explains why:

All deprecation testing in PHPStorm is not supported ATM because PHPStorm replaces the results printer and messes with the way PHPUnit tests is run already so the deprecation listener is not registered.

Thanks, that helps.

The docs are the change notice: https://www.drupal.org/node/2811561

Note that we had this for unit tests in 8.4.0, but apparently no one writes unit tests. :-)

Related to this, I built a php7.2.0 container today, and, well, its a lot ugly:

https://dispatcher.drupalci.org/job/drupalci_test_containers/837/console...

Im not sure if I should massage the 7.2.0 containers to hide deprecation notices for now, or if we just need to add in another known issue for the usages of assert to the listener.

@Mixologic those deprecations actually don't have anything to do with this issue. Those deprecations are coming from PHP they fire an unsilenced error that we definitely should not be hiding. And there's nothing our deprecation handler can do about them. Ideally we'd have had a PHP 7.2 test container before it was actually released so we could confirm that issues like #2853503: Remove all assert('string') calls from Drupal core because deprecated in PHP 7.2 are fixing the PHP7.2 issues. See https://www.drupal.org/project/issues/search?issue_tags=PHP%207.2 for other issues.

I talked with @Mixologic today in slack and the decision was to turn off deprecation fails for contrib at the infrastructure level, rather than a code change to the testbot.

We discussed that we'd rather not have minor version based changes be reflected in the testbot code (which is what #2926314: Contrib should run with --suppress-deprecations is trying to do right now).

Since the phpunit-bridge is also present in 8.4.x, it makes sense to backport the --suppress-deprecations in run-tests.sh to 8.4.x. This wouldn't have to involve a version bump for phpunit-bridge or anything more complicated than passing SYMFONY_DEPRECATIONS_HELPER=disabled or weak to the PHPUnit-based tests.

So here's a follow-up about doing that: #2927636: Backport --supress-deprecations to run-tests.sh 8.4.x

That won't be our sustainable CI, because we don't seem to have quite figured out what that is yet, have we? :-)

Added to change record https://www.drupal.org/node/2811561

Sorry for the very long delay on this. Drupalci has been cratering daily due to changes in upstream aws.

Thanks to a bunch of work by @Mile23 on this, we now have --suppress-deprecations enabled for all contrib tests.

In an ideal world, the next step to me would be to make some changes to the symfony/php-unit bridge code to make it so that it collects the deprecation data, but does not affect the test results, and outputs that deprecation data both on the command line, and with an option, as a machine readable output file that would allow for CI systems to pick out those deprecations and make a report of of them.

Then we could configure drupalci to fail the build if deprecations are detected. If Im understanding it correctly, we now have a whole list of deprecations that we are not going to detect since they are in the getSkippedDeprecations(). So we wont be made aware if people add patches that have any of those skipped deprecations in them.

Thanks, @mixologic!

IIRC, the plan is to have getSkippedDeprecations() never get bigger, and only get smaller. Someone was going to file an issue about knocking them off one by one, but I'm not sure which one that is.

The meta mentioned in #83: #2959269: [meta] Core should not trigger deprecated code except in tests and during updates

How can I ignore deprecation message appearing in my project which are removed from \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations but still exist in contrib? Should I add my own listener until contrib catches up? FWIW SYMFONY_DEPRECATIONS_HELPER is set to disabled in phpunit.xml.

@jibran are you 100% sure that SYMFONY_DEPRECATIONS_HELPER is set to disabled? Sometimes it can be tricky with env vars / phpunit.xml / phpunit.xml.dist all contributing.

@alexpott thanks, for pointing that out indeed that was the issue. Sorry, for the noise.

I'd like to suggest to update triggered error message to remove fullstop at the end of See https://www.drupal.org/node/2549395.
It make links with dot at the end so users navigate to https://www.drupal.org/node/2549395. - it works but surely pollutes page cache

Does it makes sense to file new child issue to replace all over existing places in core? (some links already has no dot at the end)

This has all long since been solved. Thanks!