Problem/Motivation

Found these while working on #3477599: Fix Drupal.Commenting.VariableComment.Missing.

Steps to reproduce

Proposed resolution

Remove the following class properties

  1. \Drupal\Tests\block\Functional\BlockUiTest::$regionsadded in #1535868: Convert all blocks into plugins, in this commit
  2. \Drupal\Tests\locale\Functional\LocaleUpdateCronTest::$batchOutput added in #1998056: Automatically update interface translations using cron, in this commit.
  3. \Drupal\Tests\user\Kernel\Views\HandlerFilterPermissionTest::$columnMap The variable was added on file creation in #1872876: Turn role permission assignments into configuration. in this commit
  4. \Drupal\KernelTests\Core\Entity\EntityCrudHookTest::$ids variable was added on file creation in #709892: Complete entity CRUD hook invocations in this commit
  5. \Drupal\Tests\system\Functional\System\PageTitleTest::$savedTitle the variable was added on file creation in #2389455: Clean-up system module test members - ensure property definition and use of camelCase naming convention in this commit
  6. \Drupal\Tests\Core\Batch\PercentagesTest::$testCases added in #2003736: Convert system module's Batch unit tests to phpunit in this commit. Actually leftover when converting to a data prodiver.
  7. \Drupal\views_ui\ViewUI::$renderPreview, variable added in #1798574: Refactor Views UI to be a form controller in this commit. Usages removed in #1983164: Entity Forms in ajax requests don't find the route, commit

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3477600

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

quietone created an issue. See original summary.

quietone opened merge request !9677

quietone’s picture

quietone commented
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review

Looks unused for real

quietone’s picture

quietone commented
Status: Needs review » Active

These appear to be unused as well.

\Drupal\Tests\user\Kernel\Views\HandlerFilterPermissionTest::$columnMap
\Drupal\KernelTests\Core\Entity\EntityCrudHookTest::$ids
\Drupal\Tests\system\Functional\System\PageTitleTest::$savedTitle
\Drupal\Tests\Core\Batch\PercentagesTest::$testCases
\Drupal\views_ui\ViewUI::$renderPreview

annmarysruthy made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Issue summary: View changes

It seems I did not same some changes to the Issue summary

quietone’s picture

quietone commented
Issue summary: View changes

@annmarysruthy, thanks for working on the MR. To complete this we also need to update the Issue Summary with some evidence that it is safe to remove these. Even if the current tests pass we want to look at the history to make sure that some testing hasn't been lost over the years.

I have started that for the 2 variables that started this, can you add the same details for the rest? I use git blame to find when the variable was added and then use the GitLab interface to add links to the commit. This information helps the reviewers and committers.

annmarysruthy’s picture

annmarysruthy commented
Issue summary: View changes
annmarysruthy’s picture

annmarysruthy commented
Issue summary: View changes
annmarysruthy’s picture

annmarysruthy commented
Issue summary: View changes
quietone’s picture

quietone commented
Issue summary: View changes

@annmarysruthy, thanks!

I am working on adding a link to the commits.

quietone’s picture

quietone commented
Title: Remove 2 unused class properties in tests » Remove 7 unused class properties in tests
Issue summary: View changes
quietone’s picture

quietone commented
Status: Active » Needs review

Time for reviews.

smustgrave’s picture

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

Seems straight forward.

Searched for some of the terms and verified where they're currently used they're actually used.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

Thanks @annmarysruthy that's awesome git archeaology. I updated one issue link and reviewed all of these.

I think we're good except for \Drupal\views_ui\ViewUI::$renderPreview because technically public properties are an API.

If we want to remove that we have to write something like \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait to use a magic __get to warn the user they're accessing a deprecated property.

APIs are forever 😢

I think that should be split into it's own issue.

Thanks everyone for the work on this one so far.

quietone’s picture

quietone commented
Status: Needs work » Needs review

I thought I commented about renderPreview.

I think we make an exception for \Drupal\views_ui\ViewUI::$renderPreview and just remove instead of deprecating it. All usages of it in core were removed in Jun 2013 before Drupal 8 was released. in this commit.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Ok I'll add a CR and commit today

quietone’s picture

quietone commented

Thanks, I started the changed record.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and published the change record - thanks!

  • larowlan committed db5dccd6 on 11.x 
    Issue #3477600 by quietone, annmarysruthy, larowlan: Remove 7 unused...

Status: Fixed » Closed (fixed)

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

quietone closed merge request !9677