Hi,

I've noticed some issues with Drupal coding standards, unused namespace imports, wrong comments, etc.
It would be nice to fix.

Comments

afi13 created an issue. See original summary.

afi13’s picture

Assigned: afi13 » Unassigned
Status: Active » Needs review
StatusFileSize
new41.2 KB
riddhi.addweb’s picture

StatusFileSize
new3.57 MB

@afi13, Thanks for solving the standard issues, I verify it with Pareview.sh. I found some file changes are missing to work upon. Can you please do the needful for the same. PFA in that I mentioned the missing files.

afi13’s picture

Assigned: Unassigned » afi13
afi13’s picture

Status: Needs review » Needs work
afi13’s picture

Status: Needs work » Needs review
StatusFileSize
new44.51 KB

README file is update also and added all required sections.

john.oltman’s picture

@afi13 appreciate the patch. The module has a README.txt and not a README.md, would you be able to post your entire README.md file as a file attachment so I can incorporate that into the commit. Thank you!

dww’s picture

Status: Needs review » Needs work

Thanks for this patch! It's fixing a lot of problems. However, it's introducing some of its own. ;) I haven't super closely reviewed everything in here, but here's what I saw on a first read:

A)

diff --git a/forward.api.php b/forward.api.php
index d2ef60f..66b6e28 100644
--- a/forward.api.php
+++ b/forward.api.php
@@ -25,7 +25,7 @@
  * @see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Utility%21token.api.php/8
  */
 function hook_forward_token(FormStateInterface $form_state) {
-  return ['my_module' => ('my_token' => 'my_value')];
+  return ['my_module' => array('my_token' => 'my_value')];
 }

Nope, we don't want array() syntax. This should be:

return ['my_module' => ['my_token' => 'my_value']];

B)

@@ -203,7 +228,11 @@ function forward_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  * Implements submit handler for hook_form_alter().
  */
 function forward_node_type_edit_form_submit($form, FormStateInterface $form_state) {
-  \Drupal::getContainer()->get('config.factory')->getEditable('forward.settings')->set('forward_node_' . $form['#node_type'], $form_state->getValue('forward_node'))->save();
+  \Drupal::getContainer()
+    ->get('config.factory')
+    ->getEditable('forward.settings')
+    ->set('forward_node_' . $form['#node_type'], $form_state->getValue('forward_node'))
+    ->save();
 }

If we're going to change this, we should simplify it to this:

\Drupal::service('config.factory')->getEditable('forward.settings')
   ->set('forward_node_' . $form['#node_type'], $form_state->getValue('forward_node'))
   ->save();

C) I know 80 character limit and all that, but I really don't think this helps readability of this logic:

-            $entity_link = $entity->access('view') ? $entity->toLink($entity->label())->toString() : t('a link');
+            $entity_link = $entity->access('view') ? $entity->toLink($entity->label())
+              ->toString() : t('a link');

D) Ditto here:

@@ -444,7 +459,8 @@ class ForwardForm extends FormBase implements BaseFormIdInterface {
 
     // Switch to anonymous user session if logged in, unless bypassing access control.
     $switched = FALSE;
-    if ($this->currentUser()->isAuthenticated() && empty($this->settings['forward_bypass_access_control'])) {
+    if ($this->currentUser()
+        ->isAuthenticated() && empty($this->settings['forward_bypass_access_control'])) {
       $this->accountSwitcher->switchTo(new AnonymousUserSession());
       $switched = TRUE;
     } 

If you really must split this line, let's do it like so:

    if ($this->currentUser()->isAuthenticated()
        && empty($this->settings['forward_bypass_access_control'])) {

E) Typo in the "fixed" version:

--- a/src/Plugin/Block/ForwardFormBlock.php
+++ b/src/Plugin/Block/ForwardFormBlock.php
@@ -10,17 +10,15 @@ namespace Drupal\forward\Plugin\Block;
...
 /**
- * Provides a block for switching users.
+ * PProvides a block with a Forward form.
  *

F) 80 character rule definitely does *not* apply to annotation comments. That's all magic, and you don't want newlines in there.

@@ -10,7 +10,8 @@ use Drupal\Core\Mail\Plugin\Mail\PhpMail;
  * @Mail(
  *   id = "forward_mail",
  *   label = @Translation("Forward HTML mailer"),
- *   description = @Translation("Sends the message as HTML, using PHP's native mail() function.")
+ *   description = @Translation("Sends the message as HTML, using PHP's native
+ *   mail() function.")
  * )
  */

Cheers,
-Derek

afi13’s picture

Assigned: afi13 » Unassigned
john.oltman’s picture

Assigned: Unassigned » john.oltman

  • afi13 authored da3125d on 8.x-2.x
    Issue #2979636 by afi13: Coding standards fixes.
    
john.oltman’s picture

Status: Needs work » Needs review
john.oltman’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

avpaderno’s picture

Issue tags: -Coding standards issue