Drupal 9 Deprecated Code Report

Here is the patch, please review.

Please review.

Thanks

Trying this again. I get the same test failure with and without this patch, so I don't think it's the patch.

Hi everyone!

Patch looks good but there are test failures. @katherined, you said at Slack that they are not related, right? What puzzles me is that https://www.drupal.org/node/20492/qa states that the last time that the test suite ran for 8.x-1.x it passed.

Triggered a rerun of the tests.

If head is broken plz first help create an issue to fix it.

What confuses me is there are no 8.8 head tests, so i guess we should update our head ci auto testbot config..

Issue to update test configurations: #3088464: Test against PHP 7 only for Drupal 8

This looks great to me. The only thing still needed is to modify the info file to require the minimum version of Drupal core which implements these things (which is where I think we introduced ConfigurableInterface). See https://www.drupal.org/node/3070687 for guidance...

Added core version requirement. Since it was introduced in 8.7.7, this makes it newer than the deprecations by about 3 minor versions and makes sense to use `^8 || ^9` as the constraint. Thoughts?

Also, is there a reason for reverting a change to remove deprecated code in #12? This prevents drupal-check from coming back clean:

```
------ -----------------------------------------------------------------------------------------------------------------------
Line src/Tests/DiffRevisionTest.php
------ -----------------------------------------------------------------------------------------------------------------------
16 Usage of deprecated trait Drupal\system\Tests\Menu\AssertBreadcrumbTrait in class Drupal\diff\Tests\DiffRevisionTest:
Scheduled for removal in Drupal 9.0.0.
Use \Drupal\Tests\system\Functional\Menu\AssertBreadcrumbTrait instead.
------ -----------------------------------------------------------------------------------------------------------------------

```

Forgot to include the version constraint in composer.json.

Yeah, I'm also confused about #12, that trait is around since 8.4 and safe to use? Accidental revert maybe?

+++ b/src/FieldDiffBuilderInterface.php
@@ -2,15 +2,15 @@
  * Builds a diff from field item list.
  */
-interface FieldDiffBuilderInterface extends PluginFormInterface, ConfigurablePluginInterface {
+interface FieldDiffBuilderInterface extends PluginFormInterface, ConfigurableInterface {
 
   /**

This change requires 8.7, so you could just as well make the jump to (only) defining core_version_requirement: ^8.7.7 || ^9 and same in composer.json

(that is still important because drupal composer facade can't yet translate core_version_requirement to a drupal/core requirement and composer might update sites with too old core versions to a new version of the module).

Ah, there is a good reason for the test change. There's a separate issue to properly convert the tests, this has incorrect base class changes without actually changing the namespace and moving them. So lets drop all test changes here and deal with that in #3089491: Convert all tests to PHPUnit.

Awesome, as per #16, the core_version_requirement has been changed to ^8.7.7 || ^9 - composer.json has also been updated to reflect this change.

The test changes still need to be reverted. So it doesn't conflict with the referenced issue.

Run all tests with both patches combined (and ignoring conflicts), I still get this warning that's relevant for this issue:

  96x: Drupal\node\Entity\Node::getRevisionAuthor is deprecated in drupal:8.2.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Entity\RevisionLogInterface::getRevisionUser() instead. See https://www.drupal.org/node/3069750
    12x in DiffPluginFileTest::testImagePlugin from Drupal\Tests\diff\Functional
    9x in DiffRevisionTest::testRevisionDiffOverview from Drupal\Tests\diff\Functional
    9x in DiffRevisionContentModerationTest::testRevisionDiffOverview from Drupal\Tests\diff\Functional
    8x in DiffRevisionTest::testRevisionOverviewErrorMessages from Drupal\Tests\diff\Functional
    8x in DiffRevisionContentModerationTest::testRevisionOverviewErrorMessages from Drupal\Tests\diff\Functional
    6x in DiffAdminFormsTest::testPluginWeight from Drupal\Tests\diff\Functional
    4x in DiffPluginTest::testTrimmingField from Drupal\Tests\diff\Functional
    3x in DiffRevisionTest::testOverviewPager from Drupal\Tests\diff\Functional
    3x in DiffRevisionContentModerationTest::testOverviewPager from Drupal\Tests\diff\Functional
    3x in NodeAccessTest::testOverview from Drupal\Tests\diff\Functional
    2x in DiffPluginVariousTest::testCorePluginTimestampField from Drupal\Tests\diff\Functional
    2x in DiffPluginVariousTest::testLinkPlugin from Drupal\Tests\diff\Functional
    2x in DiffPluginVariousTest::testCorePlugin from Drupal\Tests\diff\Functional
    2x in DiffPluginVariousTest::testTextPlugin from Drupal\Tests\diff\Functional
    2x in DiffPluginVariousTest::testTextWithSummaryPlugin from Drupal\Tests\diff\Functional
    2x in DiffPluginVariousTest::testCommentPlugin from Drupal\Tests\diff\Functional
    2x in DiffPluginTest::testApplicablePlugin from Drupal\Tests\diff\Functional
    2x in DiffPluginTest::testFieldNoAccess from Drupal\Tests\diff\Functional
    2x in DiffRevisionContentModerationTest::testEntityReference from Drupal\Tests\diff\Functional
    2x in DiffPluginTest::testFieldWithNoPlugin from Drupal\Tests\diff\Functional
    2x in DiffPluginFileTest::testFilePlugin from Drupal\Tests\diff\Functional
    2x in DiffPluginEntityTest::testEntityReferencePlugin from Drupal\Tests\diff\Functional
    2x in DiffRevisionTest::testEntityReference from Drupal\Tests\diff\Functional
    2x in DiffViewModeTest::testViewMode from Drupal\Tests\diff\Functional
    2x in DiffPluginVariousTest::testListPlugin from Drupal\Tests\diff\Functional
    1x in DiffRevisionContentModerationTest::testContentModeration from Drupal\Tests\diff\Functional

Also, tests/modules/diff_test/diff_test.info.yml needs to be updated too for the core: change.

Please review the patch.

Same here, you need to post complete patches, also this is missing the removal of the core key.

Thanks for the review. Please review the patch. If mistakes found, please let me know.

Again, when working on an issue with existing patches/work, you need to apply that first, add your changes on top of it and then provide an complete patch with all changes and an interdiff with the difference to the previous patch.

Didn't find a good documentation page but https://www.drupal.org/patch/reroll and https://www.drupal.org/documentation/git/interdiff are something you should read through. You're not really doing a reroll, but it's similar, except that there won't be conflicts.

Ok, the only thing left with this when running all tests is this:

  1x: Any entity_reference_autocomplete component of an entity_form_display must have a match_limit setting. The test_field field on the node.article.default form display is missing it. This BC layer will be removed before 9.0.0. See https://www.drupal.org/node/2863188
    1x in DiffPluginFileTest::testFilePlugin from Drupal\Tests\diff\Functional

And that isn't really going to be break in D9, just a consistency thing and requires 8.8, even if only in the test. So I think this is ready.

Tests are passing locally and all good with the manual checks. drupal-check is returning a few deprecation notices in the test files but there is nothing that can't be followed up on in a separate issue. In my opinion, this is good to go.

I'm not seeing anything with drupal-check, what are you seeing there exactly?

 ------ -------------------------------------------------------------------------------------------------------------------
  Line   src/Tests/DiffPluginEntityTest.php
 ------ -------------------------------------------------------------------------------------------------------------------
  15     Usage of deprecated trait Drupal\field_ui\Tests\FieldUiTestTrait in class Drupal\diff\Tests\DiffPluginEntityTest:
         in drupal:8.7.0 and is removed from drupal:9.0.0.
         Use \Drupal\Tests\field_ui\Traits\FieldUiTestTrait. See
         https://www.drupal.org/node/3001664
 ------ -------------------------------------------------------------------------------------------------------------------

 ------ -----------------------------------------------------------------------------------------------------------------
  Line   src/Tests/DiffPluginFileTest.php
 ------ -----------------------------------------------------------------------------------------------------------------
  17     Usage of deprecated trait Drupal\field_ui\Tests\FieldUiTestTrait in class Drupal\diff\Tests\DiffPluginFileTest:
         in drupal:8.7.0 and is removed from drupal:9.0.0.
         Use \Drupal\Tests\field_ui\Traits\FieldUiTestTrait. See
         https://www.drupal.org/node/3001664
 ------ -----------------------------------------------------------------------------------------------------------------

 ------ -----------------------------------------------------------------------------------------------------------------------
  Line   src/Tests/DiffRevisionTest.php
 ------ -----------------------------------------------------------------------------------------------------------------------
  16     Usage of deprecated trait Drupal\system\Tests\Menu\AssertBreadcrumbTrait in class Drupal\diff\Tests\DiffRevisionTest:
         in drupal:8.?.? and is removed from drupal:9.0.0.
         Use \Drupal\Tests\system\Functional\Menu\AssertBreadcrumbTrait instead.
 ------ -----------------------------------------------------------------------------------------------------------------------

 ------ ----------------------------------------------------------------------------------------------
  Line   src/Tests/DiffTestBase.php
 ------ ----------------------------------------------------------------------------------------------
  10     Class Drupal\diff\Tests\DiffTestBase extends deprecated class Drupal\simpletest\WebTestBase:
         in drupal:8.8.0 and is removed from drupal:9.0.0. Instead,
         use \Drupal\Tests\BrowserTestBase. See https://www.drupal.org/node/3030340.
  48     Call to method setUp() of deprecated class Drupal\simpletest\WebTestBase:
         in drupal:8.8.0 and is removed from drupal:9.0.0. Instead,
         use \Drupal\Tests\BrowserTestBase. See https://www.drupal.org/node/3030340.
 ------ ----------------------------------------------------------------------------------------------

 ------ -------------------------------------------------------------------------------------------------
  Line   src/Tests/DiffViewsTest.php
 ------ -------------------------------------------------------------------------------------------------
  17     Class Drupal\diff\Tests\DiffViewsTest extends deprecated class Drupal\views\Tests\ViewTestBase:
         in drupal:8.4.0 and is removed from drupal:9.0.0.
         Use \Drupal\Tests\views\Functional\ViewTestBase.
 ------ -------------------------------------------------------------------------------------------------

@Berdir This is the output I'm seeing from drupal-check for the tests.

Ah, you're missing #3089491: Convert all tests to PHPUnit, tests were fixed over there. This no longer conflicts with it as all test-related changes have been removed as they're already done.

Ready to go 👍

EDIT: Redacted. I had forgotten the difference between ^ and ~ in Composer-ese.

Committed and pushed to 8.x-1.x. Thanks!