Problem/Motivation

  • We want to have our code 100% following the Drupal coding Standards
  • In the past it was okay
  • However, has a long time that I don't verify that
  • So probably has few adjustments to be implemented

Steps to reproduce

  1. Go to Modal Page root folder
  2. Run phpcs with standards of "Drupal Coding Standards"
  3. Verify the errors that will appears

Proposed resolution

Fix all items listed to be fixed

Notes

  • Let's use the most updated version of phpcs and standards "Drupal Coding Standards" and "Drupal Best Practices"
  • Just to make sure that we're fixing to the most updated recommendations
  • In the future we can add the pipeline to verify that automatically
  • For now we can just fix manually

Note

Drupal BestPracices will be covered at [3583173]

Issue fork modal_page-3582123

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

renatog created an issue. See original summary.

renatog’s picture

renatog commented
Issue summary: View changes
renatog’s picture

renatog commented
Issue tags: +Novice
renatog’s picture

renatog commented
Parent issue: » #3553252: Plan for the next Modal Releases

vinodhini.e made their first commit to this issue’s fork.

renatog’s picture

renatog commented

@vinodhinie , I saw that you make your first commit

It's ready for review or still in progress?

Thanks for your contribution

vinodhini.e opened merge request !74

vinodhini.e’s picture

vinodhini.e commented

Hi @renatog,

Thanks for checking. I have created the MR and addressed the PHPCS and ESLint issues.

However, I am currently unable to run the pipeline to verify the changes. I’m not sure if this is due to a permission issue or something else.

Please let me know if anything needs checking from my side. Thanks.

vinodhini.e’s picture

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

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

Seems good for me! Thanks a lot @vinodhinie

renatog’s picture

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

vinodhini.e opened merge request !75

vinodhini.e’s picture

vinodhini.e commented
Status: Needs work » Needs review

Hi @renatog,

Thanks for the update.

I have followed your suggestion and created a new branch from 5.1.x. The required fixes have been reapplied, and the coding standards issues have been addressed.

A new Merge Request has been created targeting the correct 5.1.x branch.

Could you please review the updated MR.

Thanks!

renatog’s picture

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

Thank you so much @vinodhinie
:D
Now seems good

renatog’s picture

renatog commented

Merged into dev branch

@vinodhinie I saw that the fixes were applied only on JS (it's already valid)

However, do you know if on PHP codes we had issues on phpcs as well?

vinodhini.e opened merge request !76

vinodhini.e’s picture

vinodhini.e commented

@renatog,

Yes, I reviewed the PHP code as well and found several PHPCS issues. I have addressed those by applying fixes (mainly replacing \Drupal:: calls with dependency injection and resolving coding standard warnings).

I’ve now updated the code and provided a new MR with the PHPCS fixes.

Please review.. Thanks..

renatog’s picture

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

Thank you so much @vinodhinie

I'll take a look on this

Really appreciated

renatog’s picture

renatog commented
Status: Needs review » Needs work

Thanks a lot for you contribution

Seem that still having issues: 

FILE: modal_page/includes/modal_page.install.inc
-------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------------
 27 | WARNING | Do not concatenate strings to translatable strings, they should be part of the t() argument and you should use
    |         | placeholders
-------------------------------------------------------------------------------------------------------------------------------------


FILE: modal_page/modal_page.install
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 231 | ERROR | String concat is not required here; use a single string instead
-------------------------------------------------------------------------------


FILE: modal_page/src/Form/ModalForm.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 908 | ERROR | String concat is not required here; use a single string instead
-------------------------------------------------------------------------------


FILE: modal_page/src/Helper/ModalPageSettersTraitHelper.php
---------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------
 11 | ERROR | Trait names must be suffixed with "Trait"; found "ModalPageSettersTraitHelper"
---------------------------------------------------------------------------------------------


FILE: modal_page/src/Drush/Commands/ModalPageCommands.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 43 | ERROR | [x] Expected 1 blank line before function; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
renatog’s picture

renatog commented
Title: Fix 100% of Drupal Coding Standards and Best Practices » Fix 100% of Drupal Coding Standards
Issue summary: View changes
Related issues: +#3583173: Fix Drupal Best Practices for Modal 5.1.x

Created a separated issue to cover best practices, since it needs more attention on validation of dependency injection

#3583173: Fix Drupal Best Practices for Modal 5.1.x

On this one we can focus on coding standards

renatog’s picture

renatog commented
Status: Needs work » Fixed
Issue tags: -Novice

Moved to the dev branch

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.

  • 5b5875fa committed on 5.1.x 
    feat: #3582123 Fix 100% of Drupal Coding Standards

By: renatog
By:...
renatog’s picture

renatog commented

Thank you so much for your contribution @vinodhinie

Released the new version and added a public thanks for you: https://www.drupal.org/project/modal_page/releases/5.1.11

renatog’s picture

renatog commented
Title: Fix 100% of Drupal Coding Standards » Fix Drupal Coding Standards for 5.1.x