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

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

xjm created an issue. See original summary.

catch’s picture

star-szr’s picture

Title: Decide whether and how to deprecate theme functions in Drupal 8 » Complain when people use theme functions in Drupal 8 (E_DEPRECATED/assert/other)
Issue tags: +rc target triage

See #2576881-28: Deprecate theme functions for removal before Drupal 9 (docs only), repurposing this issue so retitling and tagging.

xjm’s picture

Issue summary: View changes

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

dawehner’s picture

Semantically I would totally use E_DEPRECATED because it was it is, assert() would make it impossible to use.

catch’s picture

Title: Complain when people use theme functions in Drupal 8 (E_DEPRECATED/assert/other) » [policy, no patch] Use E_DEPRECATED in Drupal 8 minor releases
Component: theme system » base system
Issue tags: -SafeMarkup, -Twig

I'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)

Crell’s picture

Title: [policy, no patch] Use E_DEPRECATED in Drupal 8 minor releases » [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases

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

catch’s picture

Issue summary: View changes
Crell’s picture

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

Crell’s picture

As 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().

dawehner’s picture

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

Well, 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.

catch’s picture

I'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.

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?

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?

dawehner’s picture

I've seen people use @trigger_error() before, not sure where.

One place I've seen it is inside \Drupal\KernelTests\KernelTestBase

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.

Okay, but do you then argue for 8.1 or 8.2?

catch’s picture

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

Crell’s picture

Also we could probably make it so that DrupalCI warns rather than fails on E_USER_DEPRECATED if we wanted to.

+1

catch, aside from the timing are you generally OK with the guidelines from #9?

catch’s picture

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

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes
Status: Active » Needs review
Issue tags: -rc target triage

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

catch’s picture

Category: Task » Plan
Crell’s picture

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

catch’s picture

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

Crell’s picture

Hm, 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. :-)

catch’s picture

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

Crell’s picture

Also true. OK, sounds like we're all on the same page.

dawehner’s picture

So should we just add a text like

Deprecation notices

In order to indicate that a certain functionality function is deprecated we use @deprecated. There
are many other usecases of deprecations (like different function parameters etc.). An additional point is the
lack of disocverability during development from @deprecated.

In order to address those problems you can use @trigger_error('Deprecation message', E_USER_DEPRECATED) in code, when the following conditions are met:

  • The deprecation is introduced as part of the next minor version.
  • All core usages are converted to the right alternative
  • the deprecation is documented via @deprecated in the previous minor branch (including documentation added in a patch release for that branch). This time period can be shorted at committer discretion, for example if using the deprecated functionality may result in functional bugs.

The deprecation notice should follow the format: trigger_error('baz() has been deprecated, use \Drupal\Foo\Bar::baz() instead. See http://drupal.org/node/the-change-notice-nid for more details.', E_USER_DEPRECATED);

E_USER_DEPRECATED can also be used to warn about deprecated features as opposed to specific functions or methods - such as triggering a deprecation notice in theme registry rebuilding when a theme function is discovered, or when a deprecated YAML key is found during discovery, or when a return value is deprecated.

to the page? This seems to be the next step on this issue.

catch’s picture

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

David_Rothstein’s picture

In order to address those problems you can use @trigger_error('Deprecation message', E_USER_DEPRECATED) in code,

The @ 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/

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gapple’s picture

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

Xano’s picture

The @ 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.

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

David_Rothstein’s picture

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Issue summary: View changes

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

/**
 * Provides a wrapper around many other services relating to entities.
 *
 * Deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. We cannot
 * use the deprecated PHPDoc tag because this service class is still used in
 * legacy code paths. Symfony would fail test cases with deprecation warnings.
 *
 * @todo Enforce the deprecation of each method once
 *   https://www.drupal.org/node/2578361 is in.
 */
class EntityManager implements EntityManagerInterface, ContainerAwareInterface {

Updating the IS.

xjm’s picture

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

catch’s picture

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

Fabianx’s picture

#34: I do agree, but that is why the Symfony phpunit bridge also has 'Unsilenced deprecations'.

So another +1 to @trigger_error().

alexpott’s picture

#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).

dawehner’s picture

Does using E_USER_DEPRECATED mean that we will add it to every existing deprecated functionality we have?

catch’s picture

Issue summary: View changes

Tried an issue summary update.

alexpott’s picture

Issue summary: View changes

Moving the new policy to a google doc so we can rapidly iterate. Link in issue summary.

Version: 8.3.x-dev » 8.4.x-dev

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

alexpott’s picture

Issue summary: View changes

I've added the core deprecation policy worked on by @catch, @dawehner, @gaborhojtsy, @xjm to our handbook https://www.drupal.org/node/2856615.

alexpott’s picture

Issue summary: View changes

Adding proposed gdo post to issue summary.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Yep, let's do this. All looks good to me.

xjm’s picture

We also need a followup meta I think to:

  1. Update the existing @deprecation notices in core to the new format, which includes the exciting work of finding the appropriate change records for them. That will require some scope management but could make a tasty novice sprint.
  2. Add the @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.
  3. And finally #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation. Some work on the implementation and process will be needed since we have lots of deprecated usages in core.

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.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Re #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.

alexpott’s picture

David_Rothstein’s picture

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

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.

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?

alexpott’s picture

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

Mile23’s picture

Seems like performing the deprecations rather than duplicating the notice would be a better use of finite resources.

Found 385 matches of @deprecated in 125 files.
catch’s picture

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?

In 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).

xjm’s picture

Status: Reviewed & tested by the community » Fixed

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

xjm’s picture

Issue summary: View changes

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

xjm’s picture

And saving issue credit.

xjm’s picture

xjm credited Gábor Hojtsy.

xjm’s picture

xjm’s picture

Ah, 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.

David_Rothstein’s picture

Sorry, 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.

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

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.

David_Rothstein’s picture

Actually, here's a compromise solution that is the best of both worlds:

  1. Keep the existing @trigger_error('...', E_USER_DEPRECATED) pattern.
  2. But also add a new setting which can be set in settings.php that nullifies the "@" and allows the messages to be displayed/logged like normal on local development sites.

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

andypost’s picture

There are 2 references to the issue in codebase, could be removed in #3261261: Properly deprecate term_access query tag