Comply with Drupal Coding Standards

Created on 25 April 2023, about 1 year ago
Updated 8 May 2023, about 1 year ago

Problem/Motivation

I have run PHPCS for this module and found the many issues.

FILE: /modules/nginx_proxy_manager_connector/src/Form/NginxProxySettingsForm.php
------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
------------------------------------------------------------------------------------------------------------------
1 | ERROR | [x] End of line character is invalid; expected "\n" but found "\r\n"
66 | ERROR | [x] Expected 1 space after "=>"; 2 found
73 | ERROR | [x] Expected 1 space after "=>"; 2 found
------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------

FILE: /modules/nginx_proxy_manager_connector/src/Services/NginxProxyManagerService.php
---------------------------------------------------------------------------------------------------------------------------------------------
FOUND 76 ERRORS AFFECTING 76 LINES
---------------------------------------------------------------------------------------------------------------------------------------------
1 | ERROR | [x] End of line character is invalid; expected "\n" but found "\r\n"
46 | ERROR | [ ] Missing parameter comment
47 | ERROR | [ ] Missing parameter comment
48 | ERROR | [ ] Missing parameter comment
49 | ERROR | [ ] Missing parameter comment
50 | ERROR | [ ] Missing parameter comment
52 | ERROR | [ ] Description for the @return value must be on the next line
54 | ERROR | [ ] Public method name "NginxProxyManagerService::create_proxy_host" is not in lowerCamel format
129 | ERROR | [ ] Missing parameter comment
130 | ERROR | [ ] Missing parameter comment
132 | ERROR | [ ] Description for the @return value is missing
134 | ERROR | [ ] Public method name "NginxProxyManagerService::deploy_proxy_host" is not in lowerCamel format
155 | ERROR | [ ] Missing parameter comment
156 | ERROR | [ ] Missing parameter comment
158 | ERROR | [ ] Description for the @return value is missing
160 | ERROR | [ ] Public method name "NginxProxyManagerService::disable_proxy_host" is not in lowerCamel format
180 | ERROR | [ ] Missing parameter comment
181 | ERROR | [ ] Missing parameter comment
182 | ERROR | [ ] Missing parameter comment
183 | ERROR | [ ] Missing parameter comment
185 | ERROR | [ ] Description for the @return value is missing
187 | ERROR | [ ] Public method name "NginxProxyManagerService::update_proxy_host" is not in lowerCamel format
232 | ERROR | [ ] Missing parameter comment
233 | ERROR | [ ] Missing parameter comment
234 | ERROR | [ ] Missing parameter comment
235 | ERROR | [ ] Missing parameter comment
237 | ERROR | [ ] Description for the @return value is missing
239 | ERROR | [ ] Public method name "NginxProxyManagerService::update_proxy_host_certificate" is not in lowerCamel format
288 | ERROR | [ ] Missing parameter comment
290 | ERROR | [ ] Description for the @return value is missing
292 | ERROR | [ ] Public method name "NginxProxyManagerService::get_proxy_host_id" is not in lowerCamel format
312 | ERROR | [ ] Missing parameter comment
314 | ERROR | [ ] Description for the @return value is missing
316 | ERROR | [ ] Public method name "NginxProxyManagerService::get_proxy_host_certificate" is not in lowerCamel format
336 | ERROR | [ ] Missing parameter comment
338 | ERROR | [ ] Description for the @return value is missing
340 | ERROR | [ ] Public method name "NginxProxyManagerService::get_certificate_id" is not in lowerCamel format
363 | ERROR | [ ] Description for the @return value is missing
365 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_init" is not in lowerCamel format
394 | ERROR | [ ] Description for the @return value is missing
396 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_health_status" is not in lowerCamel format
405 | ERROR | [ ] Description for the @return value must be on the next line
407 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_generate_token" is not in lowerCamel format
436 | ERROR | [ ] Missing parameter comment
437 | ERROR | [ ] Missing parameter comment
438 | ERROR | [ ] Missing parameter comment
439 | ERROR | [ ] Missing parameter comment
441 | ERROR | [ ] Description for the @return value must be on the next line
443 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_create_proxy_host" is not in lowerCamel format
488 | ERROR | [ ] Missing parameter comment
489 | ERROR | [ ] Missing parameter comment
490 | ERROR | [ ] Missing parameter comment
492 | ERROR | [ ] Description for the @return value must be on the next line
494 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_create_proxy_host_without_certificate" is not in lowerCamel format
539 | ERROR | [ ] Description for the @return value must be on the next line
541 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_get_proxy_hosts" is not in lowerCamel format
564 | ERROR | [ ] Missing parameter comment
566 | ERROR | [ ] Description for the @return value is missing
568 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_enable_proxy_host" is not in lowerCamel format
591 | ERROR | [ ] Missing parameter comment
593 | ERROR | [ ] Description for the @return value is missing
595 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_disable_proxy_host" is not in lowerCamel format
617 | ERROR | [ ] Missing parameter comment
618 | ERROR | [ ] Missing parameter comment
620 | ERROR | [ ] Description for the @return value must be on the next line
622 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_update_proxy_host" is not in lowerCamel format
648 | ERROR | [ ] Missing parameter comment
649 | ERROR | [ ] Missing parameter comment
651 | ERROR | [ ] Description for the @return value must be on the next line
653 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_update_proxy_host_certificate" is not in lowerCamel format
682 | ERROR | [ ] Missing parameter comment
683 | ERROR | [ ] Missing parameter comment
685 | ERROR | [ ] Description for the @return value must be on the next line
687 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_create_certificate" is not in lowerCamel format
729 | ERROR | [ ] Description for the @return value must be on the next line
731 | ERROR | [ ] Public method name "NginxProxyManagerService::api_call_get_certificates" is not in lowerCamel format
---------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------------------------------------

FILE: /modules/nginx_proxy_manager_connector/nginx_proxy_manager_connector.module
-------------------------------------------------------------------------------------------------------------------
FOUND 9 ERRORS AFFECTING 9 LINES
-------------------------------------------------------------------------------------------------------------------
1 | ERROR | [x] End of line character is invalid; expected "\n" but found "\r\n"
5 | ERROR | [x] Doc comment short description must end with a full stop
6 | ERROR | [x] Whitespace found at end of line
7 | ERROR | [x] Doc comment long description must end with a full stop
8 | ERROR | [x] Whitespace found at end of line
10 | ERROR | [x] Whitespace found at end of line
12 | ERROR | [x] Whitespace found at end of line
13 | ERROR | [x] Additional blank lines found at end of doc comment
31 | ERROR | [x] Expected 1 newline at end of file; 0 found
-------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 9 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------------

Time: 467ms; Memory: 14MB

Steps to reproduce

You can run the following command to reproduce the issue.

vendor/bin/phpcs --standard="Drupal,DrupalPractice" -n --extensions="php,module,inc,install,test,profile,theme" web/modules/nginx_proxy_manager_connector/

Proposed resolution

You can run the following command to fix error. after running this PHPCBF command some issue will fix and for the other you need to fix manually.

vendor/bin/phpcbf --standard="Drupal,DrupalPractice" -n --extensions="php,module,inc,install,test,profile,theme" web/modules/nginx_proxy_manager_connector/

Remaining tasks

User interface changes

API changes

Data model changes

📌 Task
Status

Fixed

Version

1.0

Component

Code

Created by

🇮🇳India shashank5563 New Delhi

Live updates comments and jobs are added and updated live.
  • Coding standards

    It involves compliance with, or the content of coding standards. Requires broad community agreement.

Sign in to follow issues

Comments & Activities

  • Issue created by @shashank5563
  • 🇮🇳India shashank5563 New Delhi
    headers' => [
                  'Authorization' => 'Bearer ' . \Drupal::state()->get('api_calls_token'),
                  'X-API-Version' => 'next',

    Use dependency injection for the state API.

  • 🇮🇳India shashank5563 New Delhi

    PHPCS issue fixed in this patch.

  • Status changed to Needs review about 1 year ago
  • 🇮🇳India shashank5563 New Delhi
  • Status changed to RTBC about 1 year ago
  • 🇵🇭Philippines kenyoOwen

    Hi shashank5563

    I applied your patch 3356250-3.patch → to the "Nginx Proxy Manager Connector" against Version 1.0.0-alpha1 and confirmed that the errors are resolved. Please see the screenshots attached.

    For your review.
    Thank you.

  • 🇮🇳India shashikanth171

    @shashank5563 Please create a merge request, if possible.

  • 🇮🇳India shashikanth171

    Thank you @kenyoowen for testing the patch

  • Status changed to Needs work about 1 year ago
  • 🇮🇹Italy apaderno Brescia, 🇮🇹
       * @param string $custom_domain
    +   *   Add custom domain.
        * @param string $custom_subdomain
    +   *   Add Sub Domain.
        * @param string $forward_host
    +   *   Add host.
        * @param string $port
    +   *   Add port number.
        * @param string $letsencrypt_email
    +   *   Add email for certification.

    There is no need to start each description with Add.

    -   * @return array $response
    +   * @return array
    +   *   It is returning the boolean.

    That description is not correct. If it returns an array, it is not a Boolean value. (It is spelled Boolean, not boolean.)

        * @param string $custom_domain
    +   *   Call custom domain.
        * @param int $proxy_host_id
    +   *   Define proxy host id.

    Parameter descriptions do not start with Call not Define.
    id is misspelled.

        * @return bool
    +   *   It is returning boolean

    The type of the return value is already clear. The description must say what the purpose of the return value is.

    -  public function disable_proxy_host($custom_domain, $proxy_host_id = 0) {
    +  public function disableProxyHost($custom_domain, $proxy_host_id = 0) {
         // Initiate API calls configuration.
    -    if (!$this->api_call_init()) {

    Changing the name of a public method is a disruptive change. It is better to leave that to a different issue.

    +   *   Use domain name to create proxy host.
        * @param string $forward_host
    +   *   Use host.
        * @param int $certificate_id
    +   *   Add certificate id for ssl.
        * @param string $port
    +   *   Add port number.

    None of those descriptions make sense.
    id and ssl are misspelled.

    -   * @return \Drupal\Component\Serialization\Json $json
    +   * @return \Drupal\Component\Serialization\Json
    +   *   Return the Json data.

    The description for the return value does not start with Return.

  • @shashank5563 opened merge request.
  • @shashank5563 opened merge request.
  • 🇮🇹Italy apaderno Brescia, 🇮🇹
    - * 
    - * Use create_proxy_host() to add a new proxy_host
    - * 
    - * Use deploy_proxy_host() to enable the proxy_host
    - * 
    - * Use disable_proxy_host() to disable the proxy_host
    - * 
    + *
    + * Use createProxyHost() to add a new proxy_host
    + *
    + * Use deployProxyHost() to enable the proxy_host
    + *
    + * Use disableProxyHost() to disable the proxy_host
    + *

    Since that documentation comment is changed, each sentence must end with a period.
    There is no need to add empty lines between each sentence.
    proxy_host is misspelled.

        * @param string $custom_domain
    +   *   Custom domain.
        * @param string $custom_subdomain
    +   *   Sub Domain.
        * @param string $forward_host
    +   *   Forward host.
        * @param string $port
    +   *   Port number.
        * @param string $letsencrypt_email
    +   *   Mail for certification.

    Each short description is missing an article.

    -   * @return array $response
    +   * @return array
    +   *   It is returning the Array.

    The short description must say what is returned, not its type, which is already clear from @return array.
    Array is also misspelled.

    -  public function create_proxy_host($custom_domain, $custom_subdomain, $forward_host = NULL, $port = NULL, $letsencrypt_email = NULL) {
    +  public function createProxyHost($custom_domain, $custom_subdomain, $forward_host = NULL, $port = NULL, $letsencrypt_email = NULL) {
    

    Changing names to public methods is disruptive. It is not anymore a matter of following the coding standards, but also not create issues for modules that are legitimately use public methods. These changes are rather to be considered in a separated issue.

    +   *   The action.
        * @param string $letsencrypt_email
    +   *   User email id for letsencrypt.

    It would eventually be The user email [...], but is that really a user email?
    for letsencrypt does not seem correct.

        * @param string $custom_domain
    +   *   Use the custom domain to get proxy host id.

    Short descriptions for parameters do not start with Use.
    id is misspelled, but it should not be necessary to say for which purpose is the custom domain used.

        * @return \Drupal\Component\Serialization\Json
    +   *   It is return the json data.

    It is return is grammatically not correct. (It is not even a necessary phrase.)
    json is misspelled.

        * @param string $domain_names
    +   *   Domain name to create proxy host.
        * @param string $forward_host
    +   *   Forward host.
        * @param int $certificate_id
    +   *   Certificate Id for SSL.
        * @param string $port
    +   *   Port number.

    The parameter descriptions are each missing an article.

        * @return bool
    +   *   It is return the Boolean.

    The short description must say what it is returned, not its type, which has been already said.

  • Status changed to Needs review about 1 year ago
  • 🇮🇳India shashank5563 New Delhi

    @shashikanth171, I have updated the MR (fixcodingstandard-3356250 ). Please review and merge.

  • Status changed to Needs work about 1 year ago
  • 🇮🇹Italy apaderno Brescia, 🇮🇹
        * @param string $custom_domain
    +   *  An Custom domain.
        * @param string $custom_subdomain
    +   *  An Sub Domain.
        * @param string $forward_host
    +   *  An Forward host.
        * @param string $port
    +   *  An Port number.

    I did not say which article should be used. An article means each line is missing a single article, not two or more articles.
    An is not used when the following word starts with a consonant sound.

        * @param string $letsencrypt_email
    +   *   Mail for certification.

    That description is missing an article.
    Is that description correct?

    +   * @return array
    +   *   
        */
    -  public function create_proxy_host($custom_domain, $custom_subdomain, $forward_host = NULL, $port = NULL, $letsencrypt_email = NULL) {
    +  public function createProxyHost($custom_domain, $custom_subdomain, $forward_host = NULL, $port = NULL, $letsencrypt_email = NULL) {
    

    The return value description is missing.

    -  public function deploy_proxy_host($custom_domain, $proxy_host_id = 0) {
    +  public function deployProxyHost($custom_domain, $proxy_host_id = 0) {

    Changing names to public methods is disruptive. It is not anymore a matter of following the coding standards, but also not create issues for modules that legitimately use public methods. These changes are rather to be considered in a separated issue.

        * @param string $old_custom_domain
    +   *   An Custom domain.
        * @param string $new_custom_domain
    +   *   An Custom domain.

    The same description is repeated for two different parameters. Since the parameters are different, the descriptions should be different.

        * @return bool
    +   *   

    The return value description is missing. That is what phpcs is reporting: Description for the @return value is missing.

        * @param string $custom_domain
    +   *   The custom domain to get proxy host name.

    The description is missing an article. I am not sure it is necessary to say for which purpose the custom domain is used.

        * @param string $custom_domain
    +   *   The custom domain to get certificate id.

    The description is missing an article.
    id is misspelled, since it is ID.

  • Status changed to Needs review about 1 year ago
  • 🇮🇳India shashank5563 New Delhi
  • 🇮🇳India shashikanth171

    @apaderno Are you suggesting that we can modify some functions from public to protected or private?

    Changing names to public methods is disruptive. It is not anymore a matter of following the coding standards, but also not create issues for modules that are legitimately use public methods. These changes are rather to be considered in a separated issue.

  • 🇮🇹Italy apaderno Brescia, 🇮🇹

    No, I am saying that the name of public methods cannot be simply changed because phpcs says to do so. That is a change for a different issue, which eventually must be done in a different branch.

  • 🇮🇳India shashikanth171

    @apaderno, I am changing the issue's scope to cover all Drupal coding standard related items.

    @shashank5563, please go through all code comments and correct any issues you may find.

  • 🇮🇳India shashank5563 New Delhi

    @apaderno, I have updated the MR (fixcodingstandard-3356250 ). Please review and merge.

  • Status changed to Fixed about 1 year ago
  • 🇮🇳India shashikanth171

    @shashank5563 Made some changes to the comments in the MR. Merged the MR and marking the issue Fixed.

  • Status changed to Fixed about 1 year ago
Production build 0.69.0 2024