[D8] Disable Author

Hi @mafzalzadeh,
Thank you for contribution!!

From the initial automated testing found below warnings, these are not the blockers, just good practice

Review of the 8.x-1.x branch (commit 96450d0):
The disable_author.module does not implement hook_help(). See https://www.drupal.org/docs/develop/documenting-your-project/module-docu... .
Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
DrupalPractice has found some issues with your code, but could be false positives.

FILE: ...eb/vendor/drupal/pareviewsh/pareview_temp/disable_author.routing.yml
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
 7 | WARNING | The administration page callback should probably use
   |         | "administer site configuration" - which implies the user
   |         | can change something - rather than "access administration
   |         | pages" which is about viewing but not changing
   |         | configurations.
--------------------------------------------------------------------------

Time: 140ms; Memory: 4Mb
No automated test cases were found, did you consider writing PHPUnit tests? This is not a requirement but encouraged for professional software development.
This automated report was generated with PAReview.sh, your friendly project application review script.

FILE: ...00000/site1101/web/vendor/drupal/pareviewsh/pareview_temp/README.txt
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
 12 | WARNING | Line exceeds 80 characters; contains 113 characters
--------------------------------------------------------------------------


FILE: ...101/web/vendor/drupal/pareviewsh/pareview_temp/disable_author.module
--------------------------------------------------------------------------
FOUND 7 ERRORS AFFECTING 5 LINES
--------------------------------------------------------------------------
  3 | ERROR | Missing short description in doc comment
 10 | ERROR | Missing short description in doc comment
 11 | ERROR | Missing parameter comment
 11 | ERROR | Missing parameter type
 12 | ERROR | Missing parameter comment
 13 | ERROR | Missing parameter comment
 13 | ERROR | Missing parameter type
--------------------------------------------------------------------------

Time: 398ms; Memory: 4Mb

Thanks for your contribution!

1. ConfigForm: $form['array_filter']: this element seems unused and can be removed?
2. config schema for disable_author.settings is missing, see https://www.drupal.org/docs/8/api/configuration-api/configuration-schema...
3. disable_author_form_alter(): the node type form IDs are hard-coded here? So this will only work for a "product" node type? Should it work for all node types? I think you probably want a configuration option to specify on which node types the author should be disabled?
4. disable_author_form_alter(): hide() is the wrong function to use here. You want to mark the author form element as #access => FALSE. See https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!Element!Re... . Then it will not be rendered and it is absolutely clear that access to it is denied. I think this is not a security issue but definitely wrong API usage. So I would think this is a blocker right now.

Line disable_author.module
------ ---------------------------------------------------------------------------------------------------------------
19 PHPDoc tag @param has invalid value ($route_name): Unexpected token "$route_name", expected type at offset 44
35 PHPDoc tag @param has invalid value ($form): Unexpected token "$form", expected type at offset 14
35 PHPDoc tag @param has invalid value ($form_id): Unexpected token "$form_id", expected type at offset 89
------ ---------------------------------------------------------------------------------------------------------------

[ERROR] Found 3 errors.

Fix below issues :

Shakti-Kumar:disable_author shakti.kumar$ ../../../../vendor/bin/drupal-check -ad .
2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

------ ----------------------------------
Line disable_author.module
------ ----------------------------------
6 Right side of || is always true.
------ ----------------------------------

[ERROR] Found 1 error

@shaktik It would be helpful for the OP if you could explain what exactly is wrong with the code.

Hi @kiamlaluno,

The line on 6: disable_author.module. got wrong code.

// Node author information for administrators.
    if ($form_id == 'node_product_form' ||  'node_product_edit_form') {

it should be.

  // Node author information for administrators.
    if ($form_id == 'node_product_form' || $form_id == 'node_product_edit_form') {

Thanks,
Shakti.

still have some issue.
Review of the 8.x-1.x branch (commit 96450d0):

• The disable_author.module does not implement hook_help(). See https://www.drupal.org/docs/develop/documenting-your-project/module-docu... .
• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: ...eb/vendor/drupal/pareviewsh/pareview_temp/disable_author.routing.yml
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------
   7 | WARNING | The administration page callback should probably use
     |         | "administer site configuration" - which implies the user
     |         | can change something - rather than "access administration
     |         | pages" which is about viewing but not changing
     |         | configurations.
  --------------------------------------------------------------------------
  
  Time: 140ms; Memory: 4Mb
  

• No automated test cases were found, did you consider writing PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script.

FILE: ...00000/site1101/web/vendor/drupal/pareviewsh/pareview_temp/README.txt
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
 12 | WARNING | Line exceeds 80 characters; contains 113 characters
--------------------------------------------------------------------------


FILE: ...101/web/vendor/drupal/pareviewsh/pareview_temp/disable_author.module
--------------------------------------------------------------------------
FOUND 7 ERRORS AFFECTING 5 LINES
--------------------------------------------------------------------------
  3 | ERROR | Missing short description in doc comment
 10 | ERROR | Missing short description in doc comment
 11 | ERROR | Missing parameter comment
 11 | ERROR | Missing parameter type
 12 | ERROR | Missing parameter comment
 13 | ERROR | Missing parameter comment
 13 | ERROR | Missing parameter type
--------------------------------------------------------------------------

Time: 398ms; Memory: 4Mb

FILE: ...work/d8/web/modules/contrib/disable_author/disable_author.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 43 | WARNING | There must be no blank line following an inline
    |         | comment
----------------------------------------------------------------------

Time: 37ms; Memory: 4Mb

@shaktik: small coding standard issues are not application blockers, anything else that you found or should this be RTBC?

Hi @klausi,

rest looks good, we can move RTBC.

Actually, using the wrong permission to access a route is not just a coding standards issue, but it's a security issue. The code in the 8.x-1.x branch uses the administer site configuration permission, though, which means the issue doesn't exist in the branch to review.

function disable_author_form_alter(array &$form, FormStateInterface $form_state)
{
  // Node author information for administrators.

    $config_roles = Drupal::config('disable_author.settings')
      ->get()['disallowed_roles'];
    $user = Drupal::currentUser();
    $user = User::load($user->id());
    if (array_intersect($user->getRoles(), $config_roles)) {
      $form['author'] = ['#access' => 'value', '#value' => FALSE];
      $form['created'] = ['#access' => 'value', '#value' => FALSE];
      $form['uid'] = ['#access' => 'value', '#value' => FALSE];
    }
}

The code is wrong, since:

• It's altering every form, without first checking it's the right form (which I assume should be a node edit form and not, for example, a user account edit form)
• It's replacing $form['author'], $form['created'], and $form['uid'] with an array containing two wrong items which are probably causing issues to Drupal core and third-party modules
• It's loading the full user object when the object returned from Drupal::currentUser() has a getRoles() method
• Instead of checking the roles a user has, it should check if the user has a permission

For the second point, to avoid a user sees a form element is enough to use a line similar to $form['author']['#access'] = FALSE;. An array like ['#access' => 'value', '#value' => FALSE] is going to give access to the form element, but it's also changing a form element into something that is not a form element.

1. disable_author_form_alter(): I think you forgot to check the node types here? Or where is that done? The config schema is confusing as you talk about Node there, but you check for roles?
2. module file: a couple of coding standard violations here, please fix them. See https://pareview.sh/pareview/https-git.drupal.org-project-disable_author...
3. disable_author_form_alter(): this should be documented as hook implementation, see https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

Otherwise looks good to me.

I also think we have enough code here to promote mafzalzadeh (config form, routing file, alter hook, config schema).

Thanks for your contribution, Mahmood!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Forgot to add credits for mafzalzadeh for applying here.