Problem/Motivation
Core currently has some code that is deprecated to be removed by Drupal 9.0.0, but still has hundreds of usages in core.
format_string() is an obvious example:
https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...
Proposed resolution
Have a continuous process of removing deprecated code usages.
When an API is added, we already add @trigger_error() and an @deprecated code comment explaining the deprecation. To avoid test failures, deprecations now also have to be added to the list in https://api.drupal.org/api/drupal/core%21tests%21Drupal%21Tests%21Listen... if they're being used in core.
If core is not using deprecated code, it can be removed from that skipped deprecation list [i]as long as[/i] the new API is available in a tagged release (at least alpha, stable if it's a widely used API) - this can be checked by consulting the change record for the deprecation.
In practice, a lot of our deprecations are inconsistent - some don't have trigger_error() yet for example. Therefore there are two main tasks that this issue can be used to track:
1. Add trigger_error() (and a skipped deprecation message) to @deprecated code that doesn't have it yet.
2. Where this is already in place, do the following:
- Open an issue, with a test-only patch removing the deprecation notice from the skip list, this patch should fail
- Convert all calls to the deprecated API to the new API in core, this patch should pass
- If the API is in a stable release, then the patch just needs review and commit, if not, move the test-only patch to a follow-up
Before the 9.x branch is opened, we should have an empty skip list, and no @deprecated code blocks without an associated trigger_error().
Still todo is a plan for things like constants which can't have an associated trigger_error().
Remaining tasks
Identify methods, ensure they have a sub-issue, check the status of this issue before opening the 9.x branch.
Issues:
Comment | File | Size | Author |
---|---|---|---|
#43 | 2607260-43.patch | 28.74 KB | dawehner |
Comments
Comment #2
catchComment #3
alexpottThis issue is going to spawn plenty of work during the cycle.
Comment #4
catchComment #5
xjmClarifying the title a little. I think this is a good idea.
Comment #6
xjmThis 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:
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.Comment #7
dawehnerWe 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.
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?
Comment #8
catchRemoving the usages we could technically do in patch releases, comes down to how.much code churn we want within the patch releases.
Comment #10
catchComment #12
xjmComment #15
catchRe-titling now that #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code is in Drupal 8.5.x - it's possible to accurately track deprecated code usages now so we should start removing them asap.
Comment #16
mpdonadio#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.
Comment #17
catchSo there's a long list of trigger_error() deprecations being excluded from test coverage in https://api.drupal.org/api/drupal/core%21tests%21Drupal%21Tests%21Listen...
I think we could open an issue-per-skipped message (if there's more than one message from the same class/module maybe group some together) - then we can remove the calls wherever they appear in core, as well as the message from that method in one patch.
Separately we should be retrospectively adding trigger_error() to @deprecated code when it's not already there.
Probably postpone things like constants usage until after that's cleared up.
Comment #18
BerdirCopy-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:
Comment #19
catchSo 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, tagged release can be alpha for some things but we might want more notice for others. Change records should have the branch so easy enough to look up.
Comment #20
alexpott#19 seems like an elegant and good solution to me.
Comment #21
Wim LeersTo 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
Comment #22
Wim LeersHere's a first child issue: #2922487: Follow-up for #2910211: fix all deprecation warnings.
Comment #23
catchHad a go at updating the issue summary to reflect current status.
Comment #24
alexpott@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.
Comment #25
andypostAnother solution is adoption of #2667536: Perform high level static analysis on core code
So core & contrib can track usage of deprecated stuff
Comment #26
catchI'm not sure this is a massive barrier - we'll still be removing items from the skipped list, just a few months later than adding the deprecation. A new usage might slip make it in, but it can be removed when the time comes. 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.
edit: to expand on this.
Let's say core immediately fails tests on deprecation notices for newly deprecated code. If I'm maintaining a contrib module, then I won't be able to remove the deprecated code usage and stay compatible with the current minor release of core, therefore to keep a green test suite I'd need to update my tests. Then a few months later the new API is available in a tagged release - what's going to remind me to remove the deprecation from the skiplist?
On the other hand, if we delay removing items from the skiplist in core, then contrib will get notified of the deprecation at the very time it's possible to do the update. If someone still adds it to the skiplist at that point then forgets about it until Drupal 9, then that's up to them.
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.
Comment #27
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI'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
Comment #28
dawehnerI 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.
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.
Comment #29
catchWe have probably dozens of deprecations that were done prior to 8.4.0's release that could be done at any time.
For new/recent deprecations There's no pre-beta task list, and this doesn't really work any more because beta is scheduled whether tasks are done or not. I don't think it has to happen on a schedule, just quick enough that the list doesn't grow quickly, and we should be aiming for an empty list (apart from brand new deprecations maybe) in the final 8.x minor release.
Comment #30
Wim LeersCan we do something like this?
Comment #31
catchDo you mean do that, then actually compare the version too so things automatically start failing?
Comment #32
Mile23We 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@deprecated
s 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. :-)
Comment #33
alexpott@Mile23 but the problem with:
Is that Symfony adds deprecations too - so how do we cope with that?
Comment #34
Wim Leers#31/@catch: Yes.
Comment #35
neclimdulEven that seems problematically complicated. Just asking for us to miss adding triggers.
Comment #36
Mile23Re #33:
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:
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.
Comment #37
almaudoh CreditAttribution: almaudoh commented@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.Comment #38
Mile23Convert to BrowserTestBase.
Comment #39
TR CreditAttribution: TR commentedThe 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
Comment #40
TR CreditAttribution: TR commentedOh, and in regards to #38, the change record New PHPUnit based classes added for testing: BrowserTestBase and JavascriptTestBase says quite clearly:
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.
Comment #41
Mile23From the change record for these deprecation fails: https://www.drupal.org/node/2811561
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.
Comment #42
alexpott@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.
Comment #43
dawehnerI 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:
Comment #44
alexpottNot 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?
Is twenty high enough ;)
Comment #45
catchRetitling this to match the current discussion.
Comment #46
dawehnerI 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?
Comment #47
xjmThe summary had a couple paragraphs that were duplicated verbatim, so I've removed the duplicate.
Comment #48
xjmI 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:
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.
Comment #49
xjmComment #50
neclimdulAre 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.
Comment #51
Mile23@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.
Comment #52
alexpottBut 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.
Comment #53
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThat is true, but I have learned that this is OK - see posts #5-#9 on #2924899-5: Backport \Routing\EnhancerInterface to core 8.4
Comment #54
Mile23So 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.
Comment #55
catch@Mile23 see the comments linked in #53, it's already possible to do your last paragraph if you choose 'pre-release' as the core branch for contrib project tests. We might just want to open a drupalorg issue to make that the default setting then see how things go from there?
Really the only reason to test with dev would be if you want to rely on newly introduced core APIs or an experimental module - in that case you're probably working on a new module or major revision to a module, and don't care about passing on both versions.
It's useful to have pre-alpha testing against core for unintentional bc breaks, but the false positives we'll get about the intentional deprecations at the moment is going to massively outweigh that.
Back to plan for now.
Comment #56
MixologicIf 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 newDrupal\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.
Comment #57
RoSk0Wow, that is massive. Thank you @Mixologic.
#56 Makes sense to me.
Comment #58
almaudoh CreditAttribution: almaudoh commented#56 +1000. Couldn't have put it better myself.
Comment #59
neclimdulI 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?
Comment #60
dawehner@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?
Comment #61
alexpottI 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.Comment #62
Mile23I'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:(FTR that's a run of
Drupal\Tests\file_example\Functional\FileExampleTest
after commenting out all the exceptions inDeprecationListener
.)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.Comment #63
catchThis makes loads of sense and will take the pressure off coming up with something solid here.
Comment #64
Wim LeersShould 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?
Comment #65
alexpott@Wim Leers I think we should have a setting for whether deprecations should be suppressed or not to allow contrib authors to choose.
Comment #66
Wim LeersThat makes even more sense. Allow
--suppress-deprecations
to be configured in each project's configuration.Comment #67
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented@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.
Comment #68
Mile23Added a testbot issue: #2926314: Contrib should run with --suppress-deprecations
Comment #69
Wim LeersClosed #2922537: "User deprecated function" should not cause a test to fail in the DrupalCI issue queue as a duplicate of this.
Comment #70
Wim LeersFor 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.
Comment #71
Mile23@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
.Comment #72
neclimdulHow does --suppress-deprecations impact people that use phpunit directly either via phpstorm or on the CLI?
Comment #73
Wim Leers#71: that doesn't work for me locally. Am I running tests wrong?
Comment #74
alexpott@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.
Comment #75
DamienMcKennaAs 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.
Comment #76
Wim Leers#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:Thanks, that helps.
Comment #77
Mile23The 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. :-)
Comment #78
MixologicRelated 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.
Comment #79
alexpott@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.
Comment #80
Mile23I 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
orweak
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? :-)
Comment #81
Mile23Added to change record https://www.drupal.org/node/2811561
Comment #82
MixologicSorry 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.
Comment #83
Mile23Thanks, @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.Comment #85
Mile23The meta mentioned in #83: #2959269: [meta] Core should not trigger deprecated code except in tests and during updates
Comment #87
jibranHow 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? FWIWSYMFONY_DEPRECATIONS_HELPER
is set to disabled in phpunit.xml.Comment #88
alexpott@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.
Comment #89
jibran@alexpott thanks, for pointing that out indeed that was the issue. Sorry, for the noise.
Comment #90
andypostI'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)
Comment #94
xjmThis has all long since been solved. Thanks!
Comment #95
xjm