This is a follow-up issue for #2883680: Force all route filters and route enhancers to be non-lazy
At core 8.5 \Drupal\Core\Routing\Enhancer\RouteEnhancerInterface was deprecated and replaced by \Drupal\Core\Routing\EnhancerInterface and similarly \Drupal\Core\Routing\Enhancer\RouteFilterInterface was deprecated in favour of \Drupal\Core\Routing\FilterInterface
However, the new classes are not available (yet) in 8.4 or 8.3 which is causing a knock-on effect for many contrib modules. Projects such as Rules and SMS framework cannot remove their deprecated code usage without creating a separate branch for 8.4/8.3 which is big overhead just to get tests to pass.
Other modules such as Scheduler that do not use the deprecated code still experience test failures at 8.5 due to having a test dependency on Rules.
Backport the new interface classes to Core 8.4 and 8.3
None
None
None
None
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | Screen Shot 2017-11-20 at 12.34.59 PM.png | 93.73 KB | catch |
| #2 | 2924899-2.add_routeinterface_to_8.4_and_8.3.patch | 1010 bytes | jonathan1055 |
Comments
Comment #2
jonathan1055 commentedHere is a patch which adds the replacement interface class files to 8.4 and 8.3
Comment #3
dawehnerI'm wondering whether the better approach would be to backport the capability to ignore deprecation errors and ignore those explicitly in the former branches.
Comment #4
almaudoh commentedThere is the possibility, which I haven't myself confirmed, but was mentioned by TR, that just having a module as a dependency which uses a deprecated API will cause test fails in the depending module. I'll check this when I'm done converting WTB to BTB on SMS Framework module.
#2870194-151: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code
I expect this patch to work even though I haven't had time to test it yet. Will do so later.
Comment #5
catchContrib modules can set their testing branch to 'stable', 'supported' or 'pre-release' instead of dev, which means that 8.5.x testing won't commence until either we tag 8.5.0 (for the first two) or 8.5.0-alpa1 for the last option.
Generally, unless a module is actually depending on new features added in the current dev core minor, pre-release should be the right balance of getting test failures early enough to report core bugs or update for deprecations in time for the minor release, vs late enough that it won't impact releases against the stable version. If those few weeks are still too much crossover then stable or supported will work (also possible to switch for a branch test just to see what's happening, then switch back again).
The deprecation reporting is still pretty new, and there's a discussion on making the deprecation skipping more usable here as well[#2607260].
I'd prefer to exhaust those options before we either start backporting things that are new, or talk about delaying deprecations until minor+1 and similar.
Comment #6
jonathan1055 commentedThanks @catch. So, just to work through this specific deprecation case with the real example of Rules and Scheduler contrib modules, to see if I have understood what you are saying:
@group legacyon each test (as they are using BTB, does not work for WTB)@group legacytag.With Scheduler, I have daily tests running at 8.3, 8.4 and 8.5 but I think you are saying that just 8.4 is sufficient?
I have also linked #2923267: Allow test classes to specify skipped deprecations too which would be extremely useful, as it would (a) limit the deprecation suppression to known messages added by the maintainer (so other deprecations are not hidden by accident) and (b) likely to involve fewer changed files, compared to adding the 'legacy' tag to every test class.
Comment #7
catch8.3 isn't supported since 8.4.0 came out, so there's no need to test on it any more.
When you move onto pre-release for branch tests, it'll stop testing on 8.4 unless specifically requested. As long as you don't need to make a contrib release in the time between 8.5.0-alpha1 and 8.5.0 there would be no need to test on 8.4 from that point.
Comment #8
jonathan1055 commentedThanks again @catch, that is very helpful. You have just re-vamped my understanding of the contrib testing pathway. Maybe it was documented somewhere, but it seems quite a few maintainers do not follow this, and always want all core versions tested and passing. Or only test at latest dev. There is obviously a case for checking against 8.5 currently, to discover the deprecations, but we should not mind the tests failing.
It would be great on the 'automated testing' page if maintainers could have access to a comment area where we can explain why tests fail at new versions, that it is a known problem and link to the issues which contain the pre-prepared fixes for the tech debt to be paid on the next minor release.
Comment #9
catchI'm pretty sure some of these features were added in the past year or less.
Comment #10
wim leersPer #7, 8.3 is no longer supported.
Comment #11
wim leers#5: I do think it's valuable to make it possible for modules to test against not just pre-release, but also against development. It's the maintainers of those modules who can detect unexpected/unintentional BC breaks and report back within hours/days of that change occurring. Which means that core won't have to revert potentially weeks of work.
Given that, I do also think it's valuable to always ensure that it's possible for a contrib module to stop using deprecated code against the minor (8.5 atm), and allow them to fix that in the current minor (8.4 atm).
Comment #13
borisson_I don't think 8.4 is still supported right now. That means we can close this issue.
Comment #14
tim.plunkett