Problem/Motivation

Discovered in #3581155: Investigate if shipmonk/dead-code-detector would be useful to us we have a number of unused property definitions throughout core. This issue deals with unit tests.

Steps to reproduce

Proposed resolution

Remove the unused properties.

Remaining tasks

Check why and when these properties were added in the first place.
Confirm that no test coverage has been unintentionally lost.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3581407

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

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

longwave opened merge request !15202

longwave changed the visibility of the branch 3581407-remove-unused-properties to hidden.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

I used phpstorm's find usage on these properties and they are not used according to that either. So this looks good to me, I also agree with scoping these issues like this.

catch’s picture

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

I think we're missing:

Check why and when these properties were added in the first place.
Confirm that no test coverage has been unintentionally lost.

from the issue summary.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

core/modules/language/tests/src/Unit/ContentLanguageSettingsUnitTest.php was originally added in #2355909 seems this variable was set but never used.

core/modules/migrate/tests/src/Unit/process/SubProcessTest.php was originally added in #2147811, this one was so old the ticket didn't have the commit message. But same deal variable was never used

core/modules/path_alias/tests/src/Unit/AliasManagerTest.php was originally added in #2233623 and just never used.

Same findings from #3581404: Remove unused properties from kernel tests these tests look like they haven't been touched much since added so fully believe it's dead code.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

@smustgrave, the remarks on the mr are for other properties in the same file that also might not be used.

smustgrave’s picture

smustgrave commented

Would they have been picked up by the scan with the dead code library?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I'm not sure why they wouldn't have, no. I don't think we need to hunting for more changes in this issue.

Maybe we submit these as a bug report to shipmonk?

smustgrave’s picture

smustgrave commented

Oh yea if we think it’s a bug 100% I imagine these dead code issues are about satisfying that checker only. Kinda like we do rector tickets

catch’s picture

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

Yes I think we should only handle variables picked up by shipmonk/dead-code-detector here, can open follow-ups for anything else.

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 committed 473f5e49 on main 
    task: #3581407 Remove unused properties from unit tests

By: longwave
By...

catch closed merge request !15202

Status: Fixed » Closed (fixed)

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