Problem/Motivation

I see that the module follows very old PHP standard practices, for example it doesn't follow the function return type hinting, among other features that need to be added

More information about Drupal's coding standards

Steps to reproduce

Look into the module's source code, it is using outdated PHP practices.

Proposed resolution

Upgrade the module to follow recent standard practices for writing more robust PHP code.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Issue fork many_selects-3580984

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

danrod created an issue. See original summary.

danrod’s picture

danrod
Location Ottawa
commented
Issue tags: +Novice
aayushpathak’s picture

aayushpathak commented

working on it .

aayushpathak opened merge request !18

aayushpathak’s picture

aayushpathak commented
Status: Active » Needs review
vinodhini.e’s picture

vinodhini.e commented

Hi, I tested this module on Drupal 11.2.5 and ran the Drupal coding standards (PHPCS) checks.

  • Initially, coding standard issues were reported
  • After applying MR !18, I re-ran the checks

No coding standard issues are reported now.. Thanks..

danrod’s picture

danrod
Location Ottawa
commented

I still see a couple of PHPCS issues to fix:

------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------
 37 | ERROR | [x] Multi-line function declarations must have a trailing comma after the last parameter
    |       |     (Drupal.Functions.MultiLineFunctionDeclaration.MissingTrailingComma)
 70 | ERROR | [x] Multi-line function declarations must have a trailing comma after the last parameter
    |       |     (Drupal.Functions.MultiLineFunctionDeclaration.MissingTrailingComma)
------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------

Moving this to "Needs Work" for now.

danrod’s picture

danrod
Location Ottawa
commented

Hey @5aayushpathak Could you create a MR request against the 2.0.x branch, please? I think I'll drop support for the 8.x1.x branch.

danrod’s picture

danrod
Location Ottawa
commented
Status: Needs review » Needs work

aayushpathak opened merge request !19

aayushpathak’s picture

aayushpathak commented

Hi,
I’ve created a new MR targeting the 2.0.x branch as suggested. I’ve also fixed the remaining PHPCS issues that appeared in my local environment Please review .
Thanks!

aayushpathak’s picture

aayushpathak commented
Status: Needs work » Needs review
yusuf_khan’s picture

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

Review completed for the 3580984-php-standards-2x. Static analysis (PHPStan) and coding standards (PHPCS) show no issues

vendor/bin/phpstan analyze web/modules/contrib/many_selects-3580984 --level=1 --memory-limit=1G
5/5 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

[OK] No errors

ykhan@COMM--YKHAN-MAC my-drupal-site $ vendor/bin/phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,info,txt,md,yml web/modules/contrib/vendor/bin/phpstan analyze web/modules/contrib/many_selects-3580984 --level=1 --memory-limit=1G

yusuf_khan’s picture

yusuf_khan commented
Status: Reviewed & tested by the community » Needs work

changing status to Need Works for @maintainers

danrod’s picture

danrod
Location Ottawa
commented
Status: Needs work » Needs review
danrod’s picture

danrod
Location Ottawa
commented
Issue summary: View changes

danrod changed the visibility of the branch 3580984-better-php-standerds to hidden.

danrod’s picture

danrod
Location Ottawa
commented

I checked the changes in the MR19 and the functionality is working as expected (even though It was a small text change), I'll migrate some of the changes from M18 into MR19.

danrod’s picture

danrod
Location Ottawa
commented
Status: Needs review » Reviewed & tested by the community
danrod’s picture

danrod
Location Ottawa
commented
Status: Reviewed & tested by the community » Needs work
danrod’s picture

danrod
Location Ottawa
commented

I've moved the changes from MR18 to M19 and I tested them and the widget seems to work ok, I'll move this to "Needs Review" just in case anyone wants to take an extra look on this, but I'll probably merge the changes to the 2.0.x branch later today.

danrod’s picture

danrod
Location Ottawa
commented
Status: Needs work » Needs review
danrod’s picture

danrod
Location Ottawa
commented
Status: Needs review » Reviewed & tested by the community
danrod’s picture

danrod
Location Ottawa
commented

Merging this to the 2.0.x branch.

danrod’s picture

danrod
Location Ottawa
commented

Merged, I'll create a new release out of this, thanks a lot everyone involved on this issue.

danrod’s picture

danrod
Location Ottawa
commented
Status: Reviewed & tested by the community » Fixed

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.

Status: Fixed » Closed (fixed)

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