Problem/Motivation

Contrib tests need to add to \Drupal\Tests\Listeners\DeprecationListener::getSkippedDeprecations() because we're hitting a problem where contrib can't get passing tests on the current stable branch and the dev branch because of deprecations.

Proposed resolution

Pass test class to \Drupal\Tests\Listeners\DeprecationListener::getSkippedDeprecations() and use a static property on the class to get skipped deprecations.

Remaining tasks

User interface changes

None

API changes

Addition of a param to method added in 8.5.x so not API yet.

Data model changes

None

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
8.28 KB

Here's a patch that allows tests to add to the skipped deprecations list - this should allow contrib to manage their own approach to tech debt.

jonathan1055’s picture

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

Lendude’s picture

  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?

jonathan1055’s picture

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.

Mile23’s picture

alexpott’s picture

Here's a patch that addresses #4.

I'm not wild about doing this. Something just feels like we're doing this wrong. In the rules case even though their tests are failing on 8.5.x - they are not "really" failing. They are just using deprecated code. In order to not use deprecated code the rules maintainer is going to need to release a new version that only supports 8.5.x and up. So that means a new minor that should only be tested on those versions of Drupal. I think we really need is the ability for contrib to disable/enable deprecation testing on specific branches.

jonathan1055’s picture

@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

alexpott’s picture

@jonathan1055 well you need to ability to turn off deprecation checking for the current stable branch cause that works on 8.4.x and 8.5.x and then open a new branch which is only compatible with 8.5.x and up that has deprecation checking.

almaudoh’s picture

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.

alexpott’s picture

@almaudoh the problem is we are not in charge of all deprecations - lots of the deprecations come Symfony.

jonathan1055’s picture

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.

Mile23’s picture

@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: [PP-1, 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 build.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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.