Add option to log deprecation errors

This feature is in-built in the Symfony webprofiler - the devel project exposes this but we need to take the next step and connect up some stuff. See #3005545: Collect log messages generated during a request

@Gábor Hojtsy good point. +1 on the idea to allow sites to turn on deprecation logging in some way.

And we could perhaps even report which modules are causing deprecation errors. Which might be another incentive for contrib module maintainers to solve those deprecation errors.

From the D9 chat on Monday:

mixologic
This very much seems like a good candiate for d.o. telemetry data as well.
It would be extremely useful to bridge the gap between "site logs usages of deprecated code" to reporting it back to the maintainers of the code itself, via d.o.
without requiring site owners to file issues etc.
…
I dont think we want to have issues generated. We want a log of deprecations per project
which we can expose to the project maintainers and on the project pages.

One big problem for d.o telemetry is that I'm not sure how it is going to be possible to correctly attribute who is causing the deprecation. A deprecation triggered from core code doesn't mean that core needs to change (in fact that'd never be the case because we have a zero deprecated code use policy) but I don't see a clear way to attribute the deprecation to the correct module / theme / profile. This is also a general problem for the logging. We need to make sure that if we do log we give the user enough information to file an issue we're we can triage it to go against the correct module. It is highly likely that all issues will be filed against core because that is the likely point where the deprecation has been triggered from.

@Gábor Hojtsy I agree - I've always wanted a created ticket on drupal.org button in our log viewer which would ensure that all the information we need is there. I just think #10 is mostly about the idea of automatic telemetry. Another problem that we should think about is noise. At the moment the default configuration is to keep 1000 log messages in db log but this could get extremely noisy if every request that hits the web server generates a message. Maybe we should priorities purging deprecation messages first and then limit to the count because they are the least important.

I'm curious why the same information can't be extracted using some level of static code analysis. PHPStorm and phpstan are both able to figure out this information, which would allow us to not worry about runtime issues with deprecation logging.

I think there's a reasonable amount of stuff that static code analysis can't cover, \Drupal::service()-based calls, our "optional" constructor arguments.

I don't think this is something we need *now*, we have plenty of things to work on, seems like something that sites might want to check/enable on 8.9, before making the switch to 9.0.

A log entry that says 'drupal_render() is deprecated...' is kind of meaningless in user-space. There's no error at all here, and really nothing to warn about either. It's OK to use deprecated code, which is why it's still there.

Adding a trail back to a contrib module means that someone sees this error and annoys a contrib maintainer with it. If I were still a contrib maintainer, I'd leave code where it is until the last minor release before D9 and then turn on deprecation fails. I'd do that because deprecation is a moving target, and because it's better to be compatible with old core than not. And I wouldn't want to have to explain my wisdom to people all day long because they saw it in a log.

The main justification for doing this would be if you have a pile of custom modules that don't have tests, and you want to find out if they're D9-happy. In that case, it's fair to say that the design for this issue should not surface this information beyond log entries and I guess a report on the site status page. That way you can find out which of your own custom modules represent technical debt, but you're not directed to tell a contrib maintainer what their deprecation policy should be.

@Gabor: I actually agree here with @Mile23 (/me marks day on calendar with a red cross :p), waiting for the last (or until that is in alpha, or the one before..) 8.x release seems like a perfectly valid approach to me especially for small to medium sized contrib modules. Getting "into" a module again takes some time, and it definitely has advantages when you can do that in bulk, especially since core is still catching up on the most common deprecated functions.

" promise of Drupal 8 was that we do continuous innovation and keep deprecating and adding new things so people can divide that work and not need to"
I don't think that is the main or even a very important reason for doing continuous innovation now in 8.x, the main reason IMHO is simply that we *can* do that now and at the same time, that we can *not* do that in 9.0 :)

This is a very important feature so thanks for driving this, Gabor.

Overall, I like the mockup.

The two big use cases are:

1. As a site owner, I want to see if my site can be upgraded to Drupal 9 (e.g. there should be no deprecations).
2. As a developer, I want to see what deprecations I have to remove. Because I have to manage custom code, I'd like to know the module, file name and line number of the deprecated code.

We should think through the developer workflow a bit more as it likely impacts the mockup:

• Let's say I fix a deprecation in my custom code, I'd probably want to manually 'reset' or 'mark fixed' the deprecation warnings from the screen (and hope they don't come back)?
• Let's say I upgraded a contributed module using Composer, I'd probably want to automatically 'reset' or 'clear out' the deprecation warnings for that module?

@Gabor nice UI to start with! meanwhile this deprecations should be grouped within each module versions

- CR#x1 - affected installed modules versions
 - paragraphs (1.y and later)
 - masquerade (2.y and later)
- CR#x2  - no installed modules affected

On other hand infra moving to Gitlab so it makes sense to reconsider idea #2667536: Perform high level static analysis on core code

@berdir: I actually agree here with @Mile23 (/me marks day on calendar with a red cross :p)

I still owe you $50. I hope you're coming to DrupalCon in Seattle. :-)

@Dries:

• As a site owner, I want to see if my site can be upgraded to Drupal 9 (e.g. there should be no deprecations).
• As a developer, I want to see what deprecations I have to remove. Because I have to manage custom code, I'd like to know the module, file name and line number of the deprecated code.

Added user stories to IS.

How do we trust our Symfony 4/5 or Twig 2 implementation makes sense if contribs are to update to it in the last minute when they are baked in place for good for Drupal 9 and the next chance we can change something about them without needing to support our mistakes is Drupal 10?

Mistakes!!?? Unpossible!

Right, so the actual question here is: Why didn't we run more tests? In the case of contrib, we should be showing deprecations during testing without forcing a fail because of them. We currently don't do that. I'm pretty sure @mixologic made an issue for that but I can't find it right now. That way you're a contrib dev and you have a list of things to do, but you don't have to do them right now if you don't want to.

This gets back to why we'd want this logging feature: So you can turn on logging in your old sites and find out which custom code within them needs updating. You don't need it so much in order to figure out which contrib is borked, because contrib maintainers will already be doing that testing.

Re #18: These are all features that should be in dblog. :-)

#18.3 makes me think that we might make the symfony/phpunit-bridge a non-dev requirement of core, because it does all the things you mention. I'm not sure whether that's a good idea overall, but it's already done the heavy lifting.

I agree with @Dries on the two use cases. Since this is a UI, we should assume this screen is for the site owner audience, not (necessarily) the developer audience. (For developers, some kind of command-line tool and/or something that integrates with their IDE might be more useful?)

Given it's for (at least) site owners:

- Ideally, these would be grouped by origin module/theme, not by error message. This is probably hard to do, but pointing it out, as this is the "interface" through which most site owners view their relationship with the underlying code. I did see @Mile23/@Berdir's concerns above that this may result in people annoying contrib maintainers before they're ready to make the changes, but honestly, you also get those same annoyances when you're using PHP 5 stuff that breaks on PHP 7 or what have you, so I don't really see a difference. And ideally, instead of it being annoying, it'll result in people uploading patches, and users helping each other out So You Don't Have To™. And if so, this helps with removing these deprecations gradually vs. another "Hooray! Drupal X is out! And... 13 modules are available!" moment like we're trying very hard to avoid in D9. :P
- "Since" would ideally be since Drupal 8.(minor-version) only; not even module developers will necessarily know in what arbitrary point release we decided to support Symfony 4, let alone site owners, so that seems less helpful. (I realize here you're just printing stuff verbatim from the @since message in the code, but an abstraction could be better from a UX POV.)

In other words, a "counter" UI mockup targeted at site owners could look something like this (mirroring Migrate UI, which is a screen for the same type of problem):

"View log" then takes you to the list of error messages encountered, on what files/line numbers, etc. which could either be handed to a developer, or inspected in the event that you are the developer.

We may wish to just do Gábor's from the standpoint of it being pretty easy to just knock it out, though.

Huh. NO idea why that turned out all fuzzy, but basically, start with https://www.drupal.org/files/d8-upgrade-status-review-2.PNG and work from there. Ta-da.

@webchick it looks & sounds like extension experimental module update_project and there was idea issue to add contrib modules explorer/browser to module listing

We should scope contrib D9 compatibility checking to D.O because it can always run a consistent test on commit/release and then put a badge on the project page. That test would just run the module's tests without suppressing deprecations. If the module doesn't have automated testing turned on they can't have a badge.

We'd then figure out a way to put that badge on the status/module update page on the site. The information would travel with the module update data. You'd look at your update page and it tells you which modules are free of deprecation *according to D.O*, and also gives you a way to move forward. For instance, maybe the user has NeetoModule 8.x-1.0 but 8.x-2.0 is D9 ready. The report shows you this, instead of reporting that your current module has deprecations, with no options for moving forward. In fact, your current module might be throwing deprecation errors because you patched it during deploy.

So for the scope of this issue, just adding logging of deprecation errors to dblog would be the way to go. Site owners and/or developers can then look at their current sites and find custom code that needs attention if they plan to migrate to D9. They can do this without running tests (because there probably aren't any), and they get their stacktrace to find out which module is the culprit.

hi,
i think we shouldn't rely on logged notices/errors nor on tests to ensure the module is ready for Drupal 9 :

1. notices/errors : this will not cover all the code : it should be triggered somehow to be logged : if a module has a complex if condition block and inside of it fires a drupal_set_message(), the usage of deprecated drupal_set_message() won't be logged until the complex condition is satisfied
2. tests : not all contrib have tests (nor custom projects) and we can't confirm a project is not ready for drupal9 because it has no tests

Maybe we should have a sniffer to detect all deprecated usages (a php script/ a batch ui executable script/ external library ?) : a simple way for both contrib and custom projects to collect deprecated usages everywhere

The idea of a UI for reporting what is collected is excellent : i love the #18 suggestion

It feels like there could be one dedicated issue for:

a) How would we expose this to the user?
b) What information do we log?
c) How are we able to log it in a way that the site doesn't fall apart?
For the later block I read something recently which might be an interesting solution. Let's assume we collected all the deprecations in a request and now need to store them in a database. Trigging an update/upsert query on every single request would be really bad for the actual site performance. Bloom filters could be a space and time efficient solution to figure out whether we have logged a particular deprecation already.

This needs to have some sort of filtering for the source of the error, I think.

Use case: as a site owner, I want to update deprecated usage in my codebase's custom code, but it's hard to do that if the messages about those are drowned out by all the deprecated errors coming from core and contrib!

Re: #30: (Considering how to surface deprecation info to the user on D.O):

This assumes that (a) most modules have tests (b) their tests are fairly complete to be able to deduct compatibility information from them. I am not sure both are true to the extent that this could be used to derive a badge on the project page. It would be highly misleading at best.

Yah, this is the problem, which is why I pointed out that if you don't have testing turned on you shouldn't get a badge.

Also, a compatibility test could be devised for DrupalCI to run on projects which don't have tests: Allow deprecations to fail, enable the module, and visit all the routes in the routing file. This would give you a red-light NOPE if there are deprecations and a yellow-light D9-maybe if it passes.

Adding 'needs followup' because someone should file an ideas issue about surfacing this info to the site owner role in some way other than logs.

See the UI I proposed does not say anything about Drupal 9 compatibility. @webchick is proposing such a UI display but I don't think local data gathered would be enough to deduct that information either.

That's mainly the same thing I'm reacting to in #29. We can't declare compatibility, though the 'not ready' part does track.

I like the approach of #18 because it doesn't promise anything, but it does tell you that your custom modules need some love before D9 if you can read a stacktrace. Limited scope good, more chance of happening sooner. :-)

This is awesome! Thanks for working on this everyone!

Having this solved would have been really beneficial before Drupal 9.0.0 but we still have a chance to get this done before Drupal 10.0.0... :)

I firmly believe that depredations should be caught by a static code analyzer like PHPStan. Having them logged in dev environments could be something, but that won't block CR/PR/MR from being merged with deprecated API usage.

Today I have bumped in an entity query in the theme layer (yey!) that does not add explicit access check to the query. Even if we have test coverage for the module and the UI it provides and the theme modifies, this deprecation warning is never triggered in that test because the test uses a different theme - because the module should be theme-independent.

Although, currently this deprecation is not even caught by PHPStan Drupal: https://github.com/mglaman/phpstan-drupal/issues/315

Now that PHPStan is part of Drupal core, when can we make it required to have a PHPStan that covers a deprecation before the deprecation gets introduced? Does it seem a realistic requirement to others?

Seems like a new title is in order.

What about using PHP settings to make the behavior configurable? We could make this opt-in by assuming a FALSE default and punt aggregation/filtering to a follow-up issue :)

I closed #3106098: Allow to display/log E_USER_DEPRECATED messages as a duplicate. Note that the other issue also tried to handle skipped deprecations, which is IMHO crucial for this, as some of them have been logged a _lot_ historically and would completely spam that log, making it rather useless.

Removed redundant tag

@Berdir

Note that the other issue also tried to handle skipped deprecations, which is IMHO crucial for this, as some of them have been logged a _lot_ historically and would completely spam that log, making it rather useless.

Fair, what about making those configurable via a PHP setting as well? We could hardcode a default if the setting is NULL.

Here's some initial work on this: https://git.drupalcode.org/issue/drupal-3024296/-/compare/11.x...3024296...
Not sure if filename - based deprecation level (all, contrib + custom, custom only) filtering is reliable enough, namespace is not for sure as we cannot distinguish contrib from custom at all there.

Also we could log those as ‘php_deprecation’ to distinguish from other log types.

Let's get this moving in any case :)

Created a MR against 11.x, no tests yet as it'll be nice to receive feedback first and some recommendations what'd be the best way to test (Unit?).
Marking for initial review.
Adding a patch to lock at this state to avoid composer issues if the MR gets updated as well.

MR appears to have some failures.

@smutsgrave, yes, I'm aware of that - typo in a comment. Nevertheless, I asked for a small review at this stage to ensure the direction is correct before more hours are spent on this. Can we have it?

Go for it. Haven't checked the follow up tag need either.

Thanks, received some internal feedback on this and thought about it a bit in the meantime.
TODO:
- fix typo obviously
- remove deprecation level in favor or a boolean switch and ignored file path patterns (*/core/* and */vendor/* by default)
- we should probably only log deprecations instead of displaying them on pages.

Right, now work has to be done to update existing tests and write new ones. Moving this to review again so we can see if this has chances of merging at this state assuming tests are handled.

Seems @alexpott left some feedback on the MR.

@alexpott and I have looked at this, removing the tag

Started a test but in the test there is no logging of the deprecation message. I don't see why that is. Anyone?

it may help to catch deprecations from upcoming PHP release #3427903: [META] Make Drupal 10/11 compatible with PHP 8.4

Resolved the threads but the kernel failure seems related to the change

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for system.logging with the following errors: 0 [] 'log_deprecations' is a required key., 1 [] 'deprecations_ignored_file_patterns' is a required key., 2 [] 'ignored_deprecations' is a required key.

Rebase and fixed the failing kernel test.

Believe all feedback has been addressed on this one, do we still need a follow up?

We need to do some work on the support of regular expression strings.

1. The form help text does not seem right
2. We need to validate the strings
3. The defaults are too greedy
4. The update function does not look right.