Originally a follow-up to #2572929: Document lack of auto-escape in theme functions and add a theme autoescape helper function but expanded to cover the more general case.
Problem/Motivation
Drupal 8 has numerous deprecated functions and class methods.
There is at least one deprecated function argument #2431297: Remove unused parameter from BookManagerInterface::getActiveTailIds().
Additionally there is deprecated functionality such as theme functions (which boils down to a deprecated info.yml key).
A few things to figure out:
1. We have no convention for documenting @deprecated for parameters. http://phpdoc.org/docs/latest/references/phpdoc/tags/deprecated.html does not help.
2. For some things like theme functions, we'd have completely removed support for them if we'd had time to it between removing core usages and the release candidate, and we'd like to actively discourage people from using them at all due to security concerns. E_USER_DEPRECATED could work for this.
3. For things that are just deprecated, we might also want to use E_USER_DEPRECATED - as part of the process of helping people get their modules ready for 8.x (i.e. if you can run all your tests without any E_USER_DEPRECATED, you know you're up to date with the bleeding edge of 8.x, rather than having to look through docs etc.).
Proposed resolution
Document the following (draft) as policy, https://www.drupal.org/node/2856615. This policy is based on Symfony's deprecation practices and will allow us to implement testing for deprecations.
Proposed GDO post
Updated deprecation process for Drupal 8 code
In Drupal 8, minor updates can introduce new APIs and features. When a new API is added, the old API will be deprecated in favor of the new one. We cannot remove the old API in a minor release because Drupal 8 makes a backwards compatibility promise, but it will usually be removed in the next major version (Drupal 9).
Contributed project developers, as well as those maintaining custom integrations, should follow the deprecations when possible and use the latest APIs available. This means that when Drupal 9 is released they will have to make fewer changes to be compatible.
From Drupal 8.3.0 onwards, in order to make the deprecation process as simple as possible for developers and users, we have created a new deprecation policy that details what can be deprecated and how.
Followups
- #2856742: [meta] Adopt trigger_error() for deprecation messages where it is missing
- #2856744: [META] Add trigger_error(..., E_USER_DEPRECATED) to deprecated code
- #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation
Remaining tasks
See Drupal\Core\Entity\EntityManager
:
* @todo Enforce the deprecation of each method once
* https://www.drupal.org/node/2578361 is in.
(This issue is duplicate of that one.)
User interface changes
None.
API changes
None.
Data model changes
None.
Comments
Comment #2
catch#2572929: Document lack of auto-escape in theme functions and add a theme autoescape helper function is in.
Comment #3
star-szrSee #2576881-28: Deprecate theme functions for removal before Drupal 9 (docs only), repurposing this issue so retitling and tagging.
Comment #4
xjmFor me, whether or not this can go into RC depends on how we "complain". :)
Also updating the summary since there are no more theme functions in core!
Comment #5
dawehnerSemantically I would totally use E_DEPRECATED because it was it is,
assert()
would make it impossible to use.Comment #6
catchI'm going to hijack my own issue and make this a policy/plan on E_DEPRECATED generally. Need to sort that out before we can use it for theme functions specifically.
I think we need to look at:
- deprecated functions and methods
- deprecated arguments or argument types
- deprecated data structures/features (obsolete YAML definitions)
Comment #7
Crell CreditAttribution: Crell at Palantir.net commentedSwitching to the correct constant. (E_DEPRECATED is for the engine, E_USER_DEPRECATED is for user-space code. cf http://php.net/manual/en/errorfunc.constants.php)
Comment #8
catchComment #9
Crell CreditAttribution: Crell at Palantir.net commentedE_USER_DEPRECATED (henceforth EUD), like E_NOTICE, does have a small runtime impact even if nothing happens with it, not even logging. Although we'd likely want to log it and/or display it depending on your error reporting level.
Off the cuff proposal: use E_USER_DEPRECATED if:
1) A given behavior has a clear alternative that should be used instead.
2) And that alternative existed in core prior to RC, or prior to the previous minor release. (ie, if something is marked @deprecated in 8.0.1, we don't use EUD until at least 8.2). This gives people tracking minors at least one minor release in which to update their code to remove the now-deprecated usage before it starts polluting their logs.
3) And core is no longer using it so core alone will never report an EUD.
4) And we know that it will be removed outright in 9.0 at the latest.
So, theme functions would be one such example. There's probably a bunch of other legacy core functions that should get EUD, or if adding it now would trigger EUD calls then it gives us a good indicator of what we should clean up before actually adding that call to core.
Comment #10
Crell CreditAttribution: Crell at Palantir.net commentedAs an example of #9: We want to deprecate drupal_set_message() in favor of a service. Therefore:
1) In 8.1, we introduce the service and its various pieces. drupal_set_message() is refactored to just be a stub call to the new alternative. drupal_set_message() is marked @deprecated. (Basically #2278383: Create an injectible service for drupal_set_message().)
2) In 8.2, we add trigger_error(EUD) to drupal_set_message(), including a link to instructions on using the new stuff.
3) In 9.0, we remove drupal_set_message().
Comment #11
dawehnerWell, you are using the wrong thing, so its fine to slow things down, the actual problem is what happens if this fills your log a LOT.
@Crell
I agree with your proposal, beside the 8.2 point in it. Waiting until we trigger that error, will be a long time. Just imagine we add something in january to 8.1 and then we wait for 9 months or so, to add it to 8.2? IMHO we should always use it right away, so within the next minor release. The earlier people use the right method, the better, so we want to focus on that.
One question we have to ask us though: In case we add a E_DEPRECATED, how do we deal with tests? Currently the test suite for contrib modules would also break,
as an error got thrown. I think that behaviour is totally fine, as our test suite is not part of public supported API. In case it matters, we could explore around #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation a bit more.
Comment #12
catchI've seen people use @trigger_error() before, not sure where.
The idea is that on production, you'd not fill the logs (assuming production error levels register it), but then for tests, you can use http://php.net/manual/en/book.scream.php to show all those errors.
Also we could probably make it so that DrupalCI warns rather than fails on E_USER_DEPRECATED if we wanted to.
I think that's partly a branch management question. I'd like to open the next minor release branch as soon as we get to either beta or RC for the current one, in which case we could commit the patches very close to each other in terms of clock time, but they'd get released six months apart still.
However again it depends what we're deprecating - if it's a procedural wrapper then it's not doing any harm except cruft, but also easy to update to. If it's theme functions then they're potentially harmful, but harder to update to a template. In both cases a grace period between introducing the docs and the error might be useful. However if it's something that's easy to update to, and we really think people shouldn't be using it at all, why hold off adding the error?
Comment #13
dawehnerOne place I've seen it is inside
\Drupal\KernelTests\KernelTestBase
Okay, but do you then argue for 8.1 or 8.2?
Comment #14
catchFor me there's three categories:
1. Stuff we already marked @deprecated up until and during RC - this should trigger errors asap beginning with 8.1.x
2. Stuff we 'forgot' to mark @deprecated which already has a better alternative in core which was added prior to 8.0.0 - same as #1 - except we should have some kind of window between the docs change and the error to give people time to update.
3. Stuff we add a completely new thing for (say the messages service) - I think we need to give enough time between adding the documentation, and breaking people's contrib tests - that might be a full minor release, it might be less - but it's not 0. For something like drupal_set_message() we have dozens of core usages to convert first anyway.
Comment #15
Crell CreditAttribution: Crell at Palantir.net commented+1
catch, aside from the timing are you generally OK with the guidelines from #9?
Comment #16
catch#9 is OK in general
#3 is an absolute pre-requisite to applying this anywhere. Given previous 'deprecations' that were really just wishful thinking, we need to make sure that core follows its own docs before enforcing them on anyone else.
#4 we can't 'know' anything about 9.x, except the things that we make opening the 9.x branch a condition for, and I opened #2607720: Removing 9.0.0 deprecated methods and functions last week to try to make removal of deprecated stuff (at least the easy ones) one of those things.
Comment #17
catchUpdated the issue summary with a proposed resolution, so bumping up to CNR.
Also opened #2641876: Trigger E_USER_DEPRECATED when theme functions are discovered which was the original purpose of this issue.
Comment #18
catchComment #19
Crell CreditAttribution: Crell as a volunteer commented+1 to the current summary, with one caveat: "or when a return value is deprecated." Does this mean return value from a hook or from a random function? I'm unclear where we'd trigger that... Have an example of this one?
Comment #20
catchSomething like the return value of controllers. Let's say we'd released 8.0.0 with support for string, render array, response and renderable, then we wanted to drop support for string. Every caller in core could trigger E_DEPRECATED when it gets a string back.
Comment #21
Crell CreditAttribution: Crell as a volunteer commentedHm, I see. I expect that to be an extremely rare case. Aside from controllers there are scant few places where widely varying return types aren't a design flaw to begin with. Actually if we do find any, that may be a good place to use E_USER_DEPRECATED as we fix that and give everything a single typed return. :-)
Comment #22
catchLess rare though would be dropping support for a specific routing key, or config object property which can be provided within default config. In both cases only the caller or discovery process can trigger deprecation notices, but seems reasonable to do that in either case.
Comment #23
Crell CreditAttribution: Crell as a volunteer commentedAlso true. OK, sounds like we're all on the same page.
Comment #24
dawehnerSo should we just add a text like
to the page? This seems to be the next step on this issue.
Comment #25
catchYes I think that's right.
Also I'd personally like us to start using this in 8.2.x sooner than later for things we deprecated for 8.0.x and 8.1.x.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe @ will hide the message on production sites, but will also tend to hide it on development sites which defeats a lot of the purpose.
I think a helper function would be preferable here. Then it would be easy for that function to check a configuration setting (on by default for production, off by default for development) to control whether or not the message is actually triggered. A function might also help with standardizing the format of the deprecation message.
For example, see https://developer.wordpress.org/reference/functions/_doing_it_wrong/
Comment #28
gappleI agree that the scream extension is likely unfamiliar to a lot of contrib developers, and would be a challenge to enable for many MAMP/XAMPP users.
Comment #29
XanoWhat reasons are there for not relying on PHP's own internal settings for this?
error_level
exists for a reason, and shouldn't include deprecation errors in production environment, and I can't remember the last time I saw a production environment that did.Comment #30
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented@Xano, Drupal calls
error_reporting(E_STRICT | E_ALL)
during the bootstrap (https://api.drupal.org/api/drupal/core!lib!Drupal!Core!DrupalKernel.php/...) - doesn't that override whatever the server-level settings are?I guess changing that behavior could be discussed, but it would be a big change...
Comment #32
Mile23This issue is marked as duplicate of #2578361: Discuss how to leverage E_USER_DEPRECATED
#2578361: Discuss how to leverage E_USER_DEPRECATED is referenced in a @todo in
Drupal\Core\Entity\EntityManager
:Updating the IS.
Comment #33
xjmLast week @alexpott, @effulgentsia, @Dries, @catch, @cilefen and I had an opportunity to speak to Nicolas Grekas about Symfony's BC and deprecation practices. They add this to deprecated classes/etc. in addition to marking them with
@deprecated
:@trigger_error(..., E_USER_DEPRECATED);
Right below the namespace for classes; in the appropriate code path for a method or argument. The
@
allows opting in to the notices rather than requiring sites with legacy code to opt out of them.There is also handling in DebugClassLoader::loadClass().
Symfony's PHPUnit Bridge then raises notices for deprecated code use, so developers are notified when they submit a PR. Deprecated APIs themeselves are tested in a
@legacy
code group.Comment #34
catchI think we should use the @trigger_error() for things that were proper APIs we expected people to use, and got superseded - that's a nice trick that saves flooding error logs on production that has E_DEPRECATED set. It's going to take some work to support that though, for example in web tests, even if the phpunit bridge handles it for unit tests.
However an issue like #2807705: FormattableMarkup::placeholderFormat() can result in unsafe replacements where the API was 1. never intended 2. a security risk, I think we should not suppress the error since we really want to warn people in those cases.
Comment #35
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#34: I do agree, but that is why the Symfony phpunit bridge also has 'Unsilenced deprecations'.
So another +1 to @trigger_error().
Comment #36
alexpott#2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation makes the Symfony PHPUnit bridge work for PHPUnit based Kernel and Unit tests. I think that that is a good start. We can then discuss how to implement for BrowserTestBase, JavascriptTestBase and WebTestBase (maybe we should leave WTB alone).
Comment #37
dawehnerDoes using
E_USER_DEPRECATED
mean that we will add it to every existing deprecated functionality we have?Comment #38
catchTried an issue summary update.
Comment #39
alexpottMoving the new policy to a google doc so we can rapidly iterate. Link in issue summary.
Comment #41
alexpottI've added the core deprecation policy worked on by @catch, @dawehner, @gaborhojtsy, @xjm to our handbook https://www.drupal.org/node/2856615.
Comment #42
alexpottAdding proposed gdo post to issue summary.
Comment #43
xjmYep, let's do this. All looks good to me.
Comment #44
xjmWe also need a followup meta I think to:
@trigger_error()
everywhere as appropriate. That will also be fun. I recommend doing this second since step 1 is only docs and therefor safer and 100% backportable, whereas the error-triggering could run into complications as well as best practices we haven't sorted out yet.Oh, that also reminds me. We should probably clarify the process for removing the deprecated usages in core when a new deprecation is added. That can be discussed in followup as well, once we have implementations ready to go for the phpunit bridge, because it will be much easier to sort out when trying to do it to actual issues.
Comment #45
alexpottCreated core post - https://groups.drupal.org/node/516318
Comment #46
alexpottRe #44. I think points 2 and 3 should be swapped since when we add the @trigger_error() we should also add a test for it.
Comment #47
alexpottCreated followups: #2856744: [META] Add trigger_error(..., E_USER_DEPRECATED) to deprecated code and #2856742: [meta] Adopt trigger_error() for deprecation messages where it is missing
Comment #48
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI still don't understand how this will work unless you literally only care about these showing up on the testbot. (See #26 through #30.) And even if there is a complicated way to get these to appear on the screen in my local environment, the documentation at https://www.drupal.org/node/2856615 does not explain how to do it (whether it's via the "scream" extension or otherwise).
I actually think what @Xano suggested in #29 might be a possible alternative after all. Couldn't Drupal's standard error_reporting() just exclude E_USER_DEPRECATED, but then have https://api.drupal.org/api/drupal/sites%21example.settings.local.php/8.3.x add it back? That would be much simpler.
That made it into the documentation at https://www.drupal.org/node/2856615#how-unintended, but what I don't understand is why you would use E_USER_DEPRECATED for that in the first place. If it's a security risk, shouldn't it be (at least) an E_USER_WARNING?
Comment #49
alexpott@David_Rothstein - we just need to enable it in the webprofiler module (part of the devel module) and we can have it looking like this: https://symfony.com/blog/new-in-symfony-2-2-logging-of-deprecated-calls will work.
Comment #50
Mile23Seems like performing the deprecations rather than duplicating the notice would be a better use of finite resources.
Comment #51
catchIn the case of placeholders, support for the insecure behaviour was removed entirely for 8.2.x - see https://www.drupal.org/node/2605274 The trigger_error() is triggered when an invalid placeholder is used (i.e. when the raw placeholder would be shown in the text string, instead of replaced). There's no security risk, because we chose not to keep 100% backwards compatibility in that case (because the only fully backwards compatible fix would have been insecure).
Comment #52
xjmOne thing I guess the summary does not mention is that Symfony is already using this practice quite successfully, and we are aligning our practices with theirs. Each part of the deprecation policy has a specific, sensible reason behind it (see #33), and using their pattern also makes it easy for us to adopt explicit testing for deprecation (not just "on testbot", but I also feel like #48 does not take into account how immensely value having testing integration that ensures deprecated code works correctly and is not reintroduced will be).
Also "security" is a broad category. There are hardenings, and then there are things that are only vulnerable under specific circumstances, and then there are actual exploits. Obviously, we will introduce hard breaks when necessary to fix actual exploits. But a hardening to reduce the risk of developer error causing an XSS bug in one place need not also break sites using it safely. In the case of the string fix, as @catch explained, we hardened against the vulnerability and then just warned them that strings that had previously been supported would need to be fixed to use a different placeholder strategy, as they would display unintended (but safe) content.
This practice was agreed on by Drupal 8 committers several months ago, and it was just a matter of ironing out the next steps. We've completed those next steps now, so we can mark this issue fixed. This will be the direction we're going. Thanks everyone!
Comment #53
xjmAdding the note about Symfony to the summary to properly credit them for help with how we adopted this. Thanks @fabpot and Nicolas Grekas for helping the D8 core committers with the best practices last fall!
Comment #54
xjmAnd saving issue credit.
Comment #56
xjmComment #58
xjmComment #59
xjmAh, I also updated https://www.drupal.org/node/2856615/revisions/view/10367077/10367129 to clarify that the notices are to help developers fix possibly-broken code after a BC break for unintended behavior, not as some sort of handling for actual security issues.
Comment #60
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSorry, I misread the part about security. But the point is still the same - if it's broken code (rather than just deprecated code) it should not use E_USER_DEPRECATED. I created #2860659: Broken argument replacements should trigger an E_USER_ERROR, not E_USER_DEPRECATED with a patch to fix this.
Is there a followup issue for that? My understanding is that is based on the
@deprecated
tag (not the trigger_error call) so I wonder if it only works on the usage patterns for that tag that Symfony knows about (rather than all the ways Drupal uses it). I haven't checked.Comment #61
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedActually, here's a compromise solution that is the best of both worlds:
@trigger_error('...', E_USER_DEPRECATED)
pattern.I didn't think that combination was necessarily possible, but actually it turns out it is and it is pretty simple to do. I've posted a patch in the linked issue.
Comment #63
andypostThere are 2 references to the issue in codebase, could be removed in #3261261: Properly deprecate term_access query tag