Replace deprecated RouteEnhancerInterface, then remove @group legacy

The change record is https://www.drupal.org/node/2894934
The issue is #2883680: Force all route filters and route enhancers to be non-lazy

The tests which fail are actually all in the Rules integration:

    35x in SchedulerRulesEventsTest::testRulesEvents
    14x in SchedulerRulesConditionsTest::testNodeIsScheduledConditions
    14x in SchedulerRulesActionsTest::testUnpublishOnActions
    14x in SchedulerRulesActionsTest::testPublishOnActions
    5x in SchedulerRulesConditionsTest::testNodeTypeEnabledConditions

It is not a fault in the Scheduler code. but in Rules src/Routing/RulesUiRouteEnhancer.php which uses the deprecated code.

Moving this issue to the Rules queue where I will work on a fix - and to add a test so that Rules itself will find the fault in its own code ;-)

In my Travis build, the Rules code does fail at 8.5 which is good. It is just that we have not seen any Rules patches tested at 8.5 since the commit of #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code on 9th November. Daily Rules testing is only at 8.4 currently.

To check this, here is a dummy patch which alters no code. It should fail tests at 8.5 but run OK at 8.4

As expected the current code fails at 8.5 but passes at 8.4

Here is the fix to use the new EnhancerInterface class. I am expecting this patch to pass at 8.5 but fail at 8.4

As expected, when fixing the tests to pass at 8.5 they fail at 8.4 and 8.3

When #2923267: Allow test classes to specify skipped deprecations too lands, we will be able to leave the existing deprecated code in place, add a @todo referencing this issue and list the deprecation message to be skipped in the 20 test classes that fail.

An alternative would be to try to create a \Drupal\Core\Routing\EnhancerInterface class in 8.4 and 8.3 which would use the new class but allow the same interface. Then code could be fixed and we would not have to carry forward 'technical debt' to fix in the future.

The core patch for 8.4 and 8.3 is simple and allows the fixed Rules code to pass at 8.5, 8.4 and 8.3. I have posted a patch on #2883680-112: Force all route filters and route enhancers to be non-lazy but that issue is closed so I am not certain how much visibility it will get.

Given that the new classes have not been committed to core 8.4 and 8.3 (and there is no movement or response to my patch in #112 of #2883680-112: Force all route filters and route enhancers to be non-lazy in the meantime we should be able to make the Rules tests pass by adding @group legacy to the tests. I will make a patch for this and see how it goes.

Here's a patch which add @group legacy to 16 kernel test files and 4 functional test files. Each file has:

 * @group legacy
 * @todo Remove the 'legacy' tag when Rules no longer uses deprecated code.
 * @see https://www.drupal.org/project/rules/issues/2922757

This should allow the tests to pass at core 8.5, 8.4 and 8.3

Good, as expected Rules tests pass at 8.5, 8.4 and 8.3 with this patch. Travis build results are https://travis-ci.org/jonathan1055/rules/builds/304651753 and I will create a pull-request

I've raised #2924899: Backport \Routing\EnhancerInterface to core 8.4 to discuss the problem of how Rules can remove the deprecated code without creating a new code branch and separating 8.5 development from 8.4/8.3 (which is too big a burden just to get tests to pass)

Thanks for testing TR. I will make a Pull Request for the change later today. Hopefully @fago will be able to commit this soon, but I know he does not have much time to spend on Rules.

There is some good discussion from #56 on #2607260-56: [meta] Core deprecation message introduction, contrib testing etc. where deprecation failures should be shown as a artifact of the test results and not make them fail. I think that solution is ideal.

I have created a PR for this change: https://github.com/fago/rules/pull/497

Note, the patch in #10 and PR 497 are only the first step to fixing this. When committed (hopefully soon), the issue should remain open (and postponed) until 8.5 is released and 8.6.x-dev is available. Then we no longer need to test against 8.4 and can replace the deprecated code by using the patch in #5.

In #18 fago said:

So we need to go through the 8.5 fails and fix the deprecation warnings there.

My patch in #5 does this, and fixes the rules code, but it can only be committed for 8.5+
Our tests should be run at development (8.6) pre-release (8.5) and stable (8.4) so at the moment we cannot fix the Rules code because the new EnhancerInterface is not available in core 8.4. We have to support the current stable release which is 8.4

Setting this to postponed, and when the stable release is 8.5 and we no longer need to support 8.4 we use the patch in #5 and also revert the @group legacy changes from patch #10/commit #16.

Hope that's OK with you?

Jonathan

Just for interest here is the same patch as from #5. I don't have the means at the moment to test this locally so it might not even apply any more.

Thanks for spotting that. Yes, the commit is https://cgit.drupalcode.org/rules/commit/?id=d67f785 which does pretty much the same as patch #5

Setting back to 'normal' now that this is only removing the @group legacy code. RTBC I think?

The tests at D8.5.5 were green, but they are not showing now. Sorry I changed to RTBC too early.

The test fails are unrelated. Pass at 8.5 but fail at 8.6 and 8.7. We've had 2 test fails daily from 21 March 2018 up to 13 July. Then we've had 8 test fails daily since then. I cannot see any issue raised yet to deal with these test failures.

Raised #2989050: 'No reaction rule' message uses singular form in 8.5 but plural form in 8.6 for the test failures. I don't currently have the means to test code locally (unfortunately) so I can't investigate these errors and provide patches. But at least the issue exists now.

Also added #2989417: Argument 1 passed to \EntityContext::fromEntity() must implement EntityInterface for the other test failures.

Removing the @group legacy is RTBC as the failures above are unrelated. Thanks TR.

Adding to parent