Drupal 9 Deprecated Code Report and dependency injection issues

Per a Slack discussion with Gábor Hojtsy regarding usage of D9 tags (Drupal 9, Drupal 9 compatibility, Drupal 9 readiness, etc.), "Drupal 9 compatibility" should be used for contributed projects that need updating and "Drupal 9" was the old tag for D8 issues before the D9 branch was ready. Doing tag cleanup here based on that discussion.

Assiging to myself as there is no activity from @Kushal Bansal since last few months

drupal-check report on 3080635-8.patch

../../vendor/bin/drupal-check ./
    0 [▓▓▓░░░░░░░░░░░░░░░░░░░░░░░░░]


                                                                                                                        
 [OK] No errors                                                                                                         
                                                                                                                        

Coding standard fixes. See the interdiff on next comment

1. +++ b/src/Feeds/Target/Weight.php
   @@ -2,8 +2,6 @@
   -use Drupal\feeds\Feeds\Target\Integer;
   
   @@ -14,6 +12,6 @@ use Drupal\feeds\Feeds\Target\Integer;
   -class Weight extends Integer {
   +class Weight {
   

   This changes breaks the feeds integration. This class is only loaded when the feeds module is enabled, so this changes should be reverted.

2.  ------ --------------------------------------------------------------------- 
     Line   tests/src/FunctionalJavascript/WeightTest.php                        
    ------ --------------------------------------------------------------------- 
     20     Usage of deprecated trait Drupal\field_ui\Tests\FieldUiTestTrait in  
            class Drupal\Tests\weight\FunctionalJavascript\WeightTest:           
            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                                  
    ------ --------------------------------------------------------------------- 
   

   tests/src/FunctionalJavascript/WeightTest.php still contains a call to a deprecated trait.

3.   1x: Drupal\Tests\BrowserTestBase::$defaultTheme is required in drupal:9.0.0 when using an install profile that does not set a default theme. See https://www.drupal.org/node/3083055, which includes recommendations on which theme to use.
       1x in WeightTest::testWeightSelectorBase from Drupal\Tests\weight\FunctionalJavascript
   

   in WeightTest, the defaultTheme property should be added:

4. +++ b/weight.info.yml
   @@ -3,5 +3,6 @@ name: Weight
    core: 8.x
   +core_version_requirement: ^8 || ^9
   

   Drupal 8.7 is only supported for 1 month, so I would suggest removing the core option and replace core_version_requirement with "^8.8 || ^9"

5. 'core_version_requirement' is missing in tests/modules/weight_test_views/weight_test_views.info.yml.

changes suggested by @JeroenT in #19
thanks

Looks good to me.

Thanks @JeroenT for suggestion and reviewing

Hi thanks for you work on this!

I guess there is no need to change the implementation of WeightItem::isEmpty() it is as in the core NumericItem::isEmpty and seems to make sense as Weight is a numeric field.
see https://git.drupalcode.org/project/drupal/-/blob/9.1.x/core/lib/Drupal/C...

As its not part of the drupal9 compatibility if we really need change it, we can move the optimization in a separate issue to keep the patch smaller.

Seems to be working good with Drupal 9 rc1

I'm still not sure to drop compatibility to all the version prior 8.8.

We just changed the tests basically adding `Drupal\Tests\field_ui\Traits\FieldUiTestTrait` supported from 8.6 the rest of the code work as before.

I added that line since Drupal 8.7 is only supported until Drupal 9 is released, which is in less than 2 weeks: https://www.drupal.org/core/release-cycle-overview

Also, a lot of popular modules are currently dropping Drupal 8.7 support. e.g.:

• https://git.drupalcode.org/project/paragraphs/-/blob/8.x-1.x/paragraphs....
• https://git.drupalcode.org/project/token/-/blob/8.x-1.x/token.info.yml#L4
• https://git.drupalcode.org/project/pathauto/-/blob/8.x-1.x/pathauto.info...
• https://git.drupalcode.org/project/ctools/-/blob/8.x-3.x/ctools.info.yml

People who do not want to update can stick to version 8.x-3.1.

In a first look i also saw these modules with this high drupal core requirement, but

Paragraph:
#3099355: Requirement of core version should be higher in composer.json use higher requirement just to keep compatibility

token
#3042568: Drupal 9 Deprecated Code Report for Token same

pathauto
#3042582: Drupal 9 Deprecated Code Report for Pathauto test was failing because EntityDisplayRepository::getViewDisplay() is supported only from 8.8

ctool
#3128339: Fix tests on Drupal 9 and drop support for 8.7 and #3127358: ctools block incorrectly specifies a version bumped because incompatibility with a parameter in the constructor between core versions

For us all these cases don't apply as the code still unchanged, basically here we just fixed the dependency injection (should be compatible with ^8) and tests.

I don’t get it why a module should still be installable on Drupal versions that are no longer receiving security updates. But as long as the module is installable on Drupal 9, all fine by me.

There are lot of sites that can't be upgraded to a major version and sometimes neither from one minor version to another.

If we force the compatibility to 8.8 even if this module is compatible will all the 8.x version, we stop to offer new functionalities to all these websites. Looking at the usage statistic of drupal they should be around ~60000 on a total number of ~300000 (so for weight in proportion lets say ~2000)

Of course your point is perfectly valid and probably the right thing to do. People that need this module can use a specific old version, but without any security update or new additions, with the advantage of forcing people to upgrade/update their drupal site too.

Still, we have the possibility to support also old versions so why not? We can then drop support when it's really necessary, but probably also the test compatibility is already enough?

Back to RTBC since I only updated the core_version_requirement and core property in .info.yml

Thanks @JeroenT for providing a patch.
Applied the latest patch #34 manually in Drupal 8.8 & Drupal 9, Its working for me.

Thanks everyone, pushed to 8.x-3.x.

Authored JeroenT, nitesh624 and Kushal Bansal

Could a new D9-compatible release of the module be produced before long please?

Yes, my idea was to wait until this issue automatically change to status to "closed" then release a new version.
Just to see if someone report other issues.

Looks like it's time for a new release then, unless you're aware of any relevant issues that have been reported? :-)

(Thanks!)

Released.

Thanks!