Allow test classes to specify skipped deprecations too

This looks very interesting, and would solve the problem for Scheduler module https://www.drupal.org/pift-ci-job/810669, until such time as Rules #2922757: Replace deprecated RouteEnhancerInterface, then remove @group legacy is fixed.

Is there a way that a contrib module can test these changes alongside a patch/change for it's own tests? I know you have added tests for the new functionality but it would be good to see how it works for real. If I submit a patch for a contrib module I can't see how I can include this core change in the same test build run. Any ideas?

Thanks for doing this - and making the patch so quickly,

Jonathan

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -206,6 +206,23 @@
   +   * particular test. Note, that this is means there is technical debt that
   
   +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -221,6 +221,22 @@
   +   * particular test. Note, that this is means there is technical debt that
   
   +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -274,6 +274,22 @@
   +   * particular test. Note, that this is means there is technical debt that
   

   Should be 'that this means', one 'is' too many

2. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListener.php
   @@ -35,13 +35,20 @@ public function endTest(\PHPUnit_Framework_Test $test, $time) {
   +    if (isset($test_class::$skippedDeprecations) && is_array($test_class::$skippedDeprecations)) {
   +      $skipped_deprecations = $test_class::$skippedDeprecations;
   +    }
   

   Do we want to use the same pattern we use for $modules? So check all parent classes, so you can set this in a base class and you don't have to copy it in every child class if the base class runs into deprecations during setup or something?

so you can set this in a base class and you don't have to copy it in every child class

@Lendude, that would be really helpful, because the Rules test failures are over 20 test classes.

If we're doing this then it's time to re-open #2922887: Drupal DeprecationListener only works for process-isolated tests

@alexpott

I think we really need is the ability for contrib to disable/enable deprecation testing on specific branches.

Not sure this is good, or maybe I have mis-understood. Take the Rules case as an example - to keep the tests passing we turn off deprecation failures when testing at core 8.5.x. Then we would miss new deprecations. Or are you still proposing to have the functionality to select which deprecations to ignore?

I am also not that keen on this solution, as I always want to fix problems asap and not maintain 'tech debt' carried forward. What we really want is backwards compatibility to 8.4 and in this Rules case it is simple to do - I have created a patch on #2883680-112: Force all route filters and route enhancers to be non-lazy which allows the new replacement class to be used in 8.4 and 8.3. Hopefully someone can review it, then commit and we can move forward with replacing the actual deprecated code.

Jonathan

then open a new branch which is only compatible with 8.5.x and up that has deprecation checking

@alexpott, I don't know if it is worth opening up a new branch each time an API gets deprecated, especially considering that most contrib modules are not yet using semantic versioning. So one would have to jump from 8.x-1.x to 8.x-2.x in order to avoid test fails.

IMHO, a better way would be to require that where API's are deprecated, the new API should be backported to ensure contrib modules can switch to the new API. If we don't want to backport, then the deprecation should be postponed till the next minor Drupal release.

we are not in charge of all deprecations

That is true, but many of them can be backported easily. The deprecation which caused this entire issue to be started is very easy to backport to 8.4 and 8.3, as per my patch in #2883680-112: Force all route filters and route enhancers to be non-lazy Is it possible for a Core maintainer to take a look at that, because committing this to 8.4 and 8.3 will solve the problem and we can all then move to the new non-deprecated code.

@alexpott, I don't know if it is worth opening up a new branch each time an API gets deprecated, especially considering that most contrib modules are not yet using semantic versioning.

Hey, check out this postponed issue: #1612910: [policy, no patch] Switch to Semantic Versioning for Drupal contrib extensions (modules, themes, etc)

:-)

Anyway... If contrib can specify minor versions for compatibility, then making a new release per core minor version is reasonable. But: #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning

You can specify a core branch per contrib branch in the automated testing page of a project. So an 8.x-5.x branch could map to core 8.5.x and test against it.

Also, in an ideal world, we'd be able to specify our own rules about whether deprecations fail the test in our own per-project drupalCI build file: #2901677: Allow projects to contain their own drupalci.yml file

So a lot of work is exposed by this issue, and we could either do it or just allow people to ignore deprecations in an array.

Should this be targeted for D10?

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200

Moving to PNMI to see if this is still needed? Seems like there was an issue when upgrading from 8.4 from 8.3 that I assume has been resolved as I have not heard of test issues for D10?

And it seems like this wasn't a fix a lot of people here were sure about? But could be mistaken on that.

Just following up if still valid? If no follow up could close in 3 months

Wanted to bump 1 more time before closing.

It would have been useful, but I guess if it didn't happen back then, it won't be done now. There was also not a clear agreement.
We do still have this type of problem in Contrib at 11.2 - see the list of 3rd-party modules in the summary of #3514380: Autoloading hooks in the file tokens.inc is deprecated D11.2 which clouds the log with noise and affects the test results.
But thanks for following up, though. If no one else picks this up it can be closed.

Well I guess since the introduction of the .deprecation-ignore.txt file this is solved in the sense that contrib can define their own file, and use it during testing with a proper setup of the SYMFONY_DEPRECATIONS_HELPER environment variable, no?

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/phpunit.xml.d...

Thanks mondrake, yes you are right. I had forgotten that solution, and have updated the issue summary. This can be closed now.

Per #31