Problem/Motivation

We skip a number of deprecations in .deprecation-ignore.txt but some of these are outdated or could be tightened up.

Also, there's a number of tests that are tagged with the #[IgnoreDeprecations] attribute, but no deprecations are actually triggered during the test execution.

Proposed resolution

  • remove obsolete lines from .deprecation-ignore.txt
  • remove #[IgnoreDeprecations] from tests that do not trigger deprecations
  • add @phpstan-ignore annotations to prevent PHPStan errors in code that by removing #[IgnoreDeprecations] is no longer resolved as deprecated scope

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3576579

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !14960

mondrake changed the visibility of the branch 3576579-tidy-up-and to hidden.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3523614: [CI] Collect and report deprecation statistics and details

The stats to support the cleanup are coming from the WIP #3523614: [CI] Collect and report deprecation statistics and details.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes

Updated IS

dcam’s picture

dcam commented
Status: Needs review » Needs work

I reviewed the changes and didn't find anything to comment on. So I did my own check to see if there were additional tests from which we can remove #[IgnoreDeprecations]. I found the following 6 files:

  • core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php
  • core/modules/system/tests/src/Kernel/Common/AlterTest.php
  • core/tests/Drupal/KernelTests/Core/Datetime/DatetimeElementFormTest.php
  • core/tests/Drupal/KernelTests/Core/Hook/HookAlterOrderTest.php
  • core/tests/Drupal/KernelTests/Core/Hook/HookCollectorPassTest.php
  • core/tests/Drupal/KernelTests/Core/Hook/HookOrderTest.php

DatetimeElementFormTest already has one removal, but removing the other one didn't cause a problem.

I'm guessing these weren't simply missed by earlier checks. Probably they were made possible by recent deprecation removal commits. Four of those relate to Core libraries. I know we had one or two patches land that affected those classes in the last few days.

I'm setting the status to Needs Work so my findings can be double-checked and possibly added to the MR.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

#8 thanks! Yes, done. BTW it was also confirmed rebasing #3523614: [CI] Collect and report deprecation statistics and details and getting fresh stats https://git.drupalcode.org/issue/drupal-3523614/-/jobs/8787171, which is a good sign that one is doing its job

mondrake’s picture

mondrake
Location 🇮🇹
commented

Reverted last commit, it was driven by false negatives in #3523614: [CI] Collect and report deprecation statistics and details

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

This one needs a rebase, imagine when your testing framework cleanup ticket lands too so not sure if you want to wait?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

This is not hard to reroll; thanks for asking. Rebased

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs review

Oops sorry it wasn't RTBC

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

if you don't mind then I don't mind marking. Seems like a good chunk.

catch’s picture

catch
he/him
Primary language English
commented

One question on the MR.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Well, I think we came across a misconcept here, courtesy of #3523614: [CI] Collect and report deprecation statistics and details.

If the purpose of the test is, per doc,

make sure installing the deprecated module doesn't trigger a deprecation notice

then we are not expecting deprecations here. And in fact there is no deprecation expectation in the test. That means it's right to remove the attribute, that prevents tests to fail if deprecations are triggered. And in fact the referenced issue shows that none occur.

  • catch committed 8c44045f on main 
    task: #3576579 Tidy up and tighten deprecation ignores and tests marked...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Oh I see what you mean. The wording is essentially the opposite of what the attribute does, I think I was mentally adding 'doesn't cause a test failure' to the end of the sentence.

Committed/pushed to main, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !14960

Status: Fixed » Closed (fixed)

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