Coding Standards Issues and Deprecations in codebase [#3262956]

Problem/Motivation

The following coding standards and code deprecations are present in the codebase:

Coding Standards:

FILE: ...odules/contrib/advancedqueue/src/Plugin/views/field/JobState.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
49 | ERROR | [x] Use null coalesce operator instead of ternary
| | operator.
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...advancedqueue_test/src/Plugin/AdvancedQueue/JobType/Flexible.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
31 | ERROR | [x] Use null coalesce operator instead of ternary
| | operator.
32 | ERROR | [x] Use null coalesce operator instead of ternary
| | operator.
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...eue/tests/modules/advancedqueue_test/advancedqueue_test.info.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
1 | WARNING | "Description" property is missing in the info.yml file
----------------------------------------------------------------------

Time: 1.73 secs; Memory: 14MB

Drupal Deprecations introduced in 9.0 and 9.1:

Remaining self deprecation notices (42)

22x: Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407
8x in ProcessorTest::testProcessor from Drupal\Tests\advancedqueue\Kernel
8x in ProcessorTest::testRetry from Drupal\Tests\advancedqueue\Kernel
6x in ProcessorTest::testTimeLimit from Drupal\Tests\advancedqueue\Kernel

4x: Declaring ::setUp without a void return typehint in Drupal\Tests\advancedqueue\Kernel\ProcessorTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724
4x in DrupalListener::startTest from Drupal\Tests\Listeners

4x: The Drupal\Tests\advancedqueue\Kernel\ProcessorTest::$modules property must be declared protected. See https://www.drupal.org/node/2909426
4x in DrupalListener::startTest from Drupal\Tests\Listeners

3x: Declaring ::setUp without a void return typehint in Drupal\Tests\advancedqueue\Functional\QueueTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724
3x in DrupalListener::startTest from Drupal\Tests\Listeners

3x: The Drupal\Tests\advancedqueue\Functional\QueueTest::$modules property must be declared protected. See https://www.drupal.org/node/2909426
3x in DrupalListener::startTest from Drupal\Tests\Listeners

3x: Declaring ::setUp without a void return typehint in Drupal\Tests\advancedqueue\Kernel\DatabaseBackendTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724
3x in DrupalListener::startTest from Drupal\Tests\Listeners

3x: The Drupal\Tests\advancedqueue\Kernel\DatabaseBackendTest::$modules property must be declared protected. See https://www.drupal.org/node/2909426
3x in DrupalListener::startTest from Drupal\Tests\Listeners

Steps to reproduce

1. Scan codebase with phpcs using Drupal Coding Standards. (coding standards issues)
2. Execute module's unit tests with PHP 7 or higher, using Drupal 9.1 or higher as a base install.

Proposed resolution

Patch to address both codings standards and Drupal deprecations is attached. This patch will remove support for Drupal 8, which was deprecated in November 2021.

Comment	File	Size	Author
	drupal-9-deprecations.patch	5.42 KB	lisa.rae

Issue fork advancedqueue-3262956

Show commands

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

Add & fetch this issue fork’s repository

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

Add & fetch this issue fork’s repository

3262956-coding-standards-issues changes, plain diff MR !5
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

7 February 2022 at 17:56

lhridley created an issue. See original summary.

Comment #2

lisa.rae commented 7 February 2022 at 17:57

Status:

Active

» Needs review

Comment #3

benjifisher

he/him or they/them

English

Boston area

commented 8 February 2022 at 03:40

Status:

Needs review

» Needs work

@lhridley:

Thanks for working on this issue!

When you upload a new patch, please follow the Patch naming conventions. Usually, when I have already done the work before opening an issue, I create the issue so that I know the issue number, then create and upload the patch.

Alternatively, create a merge request (MR) instead of a patch. See Creating issue forks and merge requests. One advantage of a MR is that it gives an easy way to test the changes with an actual Drupal 10 site: see Testing A Drupal 8 Module On Drupal 9 in the Fruition blog.

If you attach a patch to Comment #4, then the recommended file name will be something like advancedqueue-drupal-10-compatibility-3262956-4.patch. Or at least 3262956-4.patch.

I ran phpcs and also used the report from the Upgrade Status module before and after applying the patch. I also reviewed the patch manually, and I ran the module's PHPUnit tests. I have just three comments:

```
+++ b/advancedqueue.info.yml
@@ -2,7 +2,7 @@ name: 'Advanced Queue'
 type: module
 description: 'Provides a better Queue API.'
 package: Other
-core_version_requirement: ^8.7.7 || ^9
+core_version_requirement: ^9.1
```
After this change, the module would still not be compatible with Drupal 10. I think we should use ^9.1 || ^10. For the record: we need 9.1 and not just 9 because Drupal\Component\EventDispatcher\Event was added in Drupal 9.1.

+++ b/src/Processor.php
@@ -103,8 +103,7 @@ class Processor implements ProcessorInterface {
     // Update the job with the result.
     $job->setState($result->getState());
     $job->setMessage($result->getMessage());
-
-    $this->eventDispatcher->dispatch(AdvancedQueueEvents::POST_PROCESS, new JobEvent($job));
+    $this->eventDispatcher->dispatch(new JobEvent($job), AdvancedQueueEvents::POST_PROCESS);

Really minor point: removing the blank line makes the diff a little harder to compare (at least the way I review diffs).

I am not sure that the coding standards issues need to be fixed. But that affects only 3 lines in 2 files, so it is not a big deal.

The Upgrade Status report also complains about the three classes in the Drupal\advancedqueue\Command namespace, which use Drupal Console classes (Drupal\Console namespace). I think it is safe to leave these classes where they are, but one of us should add an issue to convert them to Drush commands, unless Drupal Console releases a version compatible with Drupal 10.

Comment #4

benjifisher

he/him or they/them

English

Boston area

commented 8 February 2022 at 03:58

This patch will remove support for Drupal 8, which was deprecated in November 2021.

True. As #3.1 and #3248140-4: Symfony 4.4 event dispatcher parameter order change point out, the changes here require at least Drupal 9.1, so there is no point in claiming D8 compatibility in the .info.yml file.

It might make sense for the maintainers to open a new branch (8.x-2.x or 2.0.x) including a fix for this issue, so that they can continue working on a full release to support those sites that are still using D8 after EOL while also working towards D10.

Looking at the issue queue, I see about 2 dozen issues for the 8.x-1.x branch, but

None of these issues are marked Major (nor Critical).
Most are feature requests. Only 3 are bugs.

I see there is already a roadmap issue: #3220292: Roadmap to 1.0 lists just 3 issues (NR, NR, NW) standing in the way of a full release.

Comment #5

lisa.rae commented 8 February 2022 at 13:40

Thanks for reviewing @benjifisher!

Naming conventions for patch: Apologies, forgot to rename the patch before submitting it. Duly noted.

Using MR vs Patch: Completely slipped my mind that this was an option. Until additional refactoring this module fails testing with Drupal 10 (see notes below on D 10 compatibility), primarily because you need Drush 11 in place for Drupal 10 (at least, my testing refused to install any modules on D10 without Drush 11).

Drupal 10 compatibility: This patch was not intended to address Drupal 10 compatibility, but to fix the deprecation notices and coding standards issues present in the current Drupal 9 codebase. There are additional items to refactor to make the project Drupal 10 compatible, including removing the Drupal Console commands and fixing some issues with the Drush commands that are due to deprecated functionality in Drush that has been removed in Drush 11.

Removing support for Drupal 8: I specifically called this out so that the module maintainers could make a decision on whether to create a new branch.

Drupal Console: Given that the project has more or less been abandoned (no work since November 2019) in favor of Drush, this refactoring should be either done independently, or in conjunction with refactoring for Drupal 10 compatibility.

Comment #6

lisa.rae commented 8 February 2022 at 14:18

Assigned:

Unassigned

» lisa.rae

Comment #7

victoria-marina commented 9 February 2022 at 19:36

Assigned:

lisa.rae

» victoria-marina

Awaiting the new changes to review.

Comment #8

victoria-marina commented 9 February 2022 at 20:39

Assigned:

victoria-marina

» Unassigned

Comment #9

13 February 2022 at 18:04

lhridley opened merge request !5

Comment #10

lisa.rae commented 13 February 2022 at 18:06

Comment #11

lisa.rae commented 13 February 2022 at 18:09

Status:

Needs work

» Needs review

MR created on Gitlab, replaces original submitted patch.

Please note related issue removing Drupal Console commands. This has been opened as a separate issue to give any users who use Drupal Console to check and replace any functionality that removing those commands removes from the module with Drush commands that provide replacement functionality.

Comment #12

victoria-marina commented 14 February 2022 at 17:42

Assigned:

Unassigned

» victoria-marina

I'll review this.

Comment #13

victoria-marina commented 14 February 2022 at 17:54

Assigned:	victoria-marina	» Unassigned
Status:	Needs review	» Reviewed & tested by the community

After the last commit, the EventDispatcher::dispatch() argument order was correctly changed.

Comment #14

12 April 2022 at 18:05

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

Comment #15

12 April 2022 at 18:06

bojanz committed bb7d1a0 on 8.x-1.x authored by lhridley

#3262956: Address code deprecations and coding standards for Drupal 9.1+

Comment #16

bojanz commented 12 April 2022 at 18:08

Status:

Reviewed & tested by the community

» Fixed

Great job, everyone!

Comment #17

26 April 2022 at 18:09

Status:

Fixed

» Closed (fixed)

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

Coding Standards Issues and Deprecations in codebase

Problem/Motivation

Steps to reproduce

Proposed resolution

Issue fork advancedqueue-3262956

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Related issues

Referenced by

News items

Our community

Documentation

Drupal code base

Governance of community