[1.0.x] SmartLink Formatter

Thank you for applying!

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review → gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

Has the account used for this application been created by a person who already has another account?

I have only once account
How you need info from my end?

Fix phpcs issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml smartlink_form
atter/

FILE: smartlink_formatter/smartlink_formatter.info.yml                                                    
------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE                                                                                                            
------------------------------------------------------------------------------
 7 | ERROR | [x] Expected 1 newline at end of file; 0 found                                                                               
------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY                                                                                
------------------------------------------------------------------------------


FILE: smartlink_formatter/src/Plugin/Field/FieldFormatter/SmartLinkFormatter.php                          
------------------------------------------------------------------------------
FOUND 0 ERRORS AND 19 WARNINGS AFFECTING 19 LINES                                                                                         
------------------------------------------------------------------------------
 100 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead 
 101 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead 
 104 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead 
 108 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead 
 114 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead 
 137 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead 
 148 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 159 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 170 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 192 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 195 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 199 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 202 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 206 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 209 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 212 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 215 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 218 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 242 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
------------------------------------------------------------------------------

Hello @vishal
I have fixed those above-mentioned phpcs issues.
Please review

FILE: src/Plugin/Field/FieldFormatter/SmartLinkFormatter.php

$link_title = \Drupal::token()->replace($item->title, [$entity->getEntityTypeId() => $entity], ['clear' => TRUE]);

\Drupal calls should be avoided in classes, use dependency injection instead

@vishal I have applied the changes as per Drupal standard code for token service.
Please review

Rest looks fine to me.

Let’s wait for a Code Review Administrator to take a look and if everything goes fine, you will get the role.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that doesn't follow the coding standards, contains possible security issue, or does not correctly use the Drupal API; the single points are not ordered, not even by importance

src/Plugin/Field/FieldFormatter/SmartLinkFormatter.php

This is essentially a copy of the LinkFormatter class that has been changed to include more attributes. That is fine, and it is preferable to use as base class a class that is not part of the public API, contrary to base classes like FormatterBase.

Still, SmartLinkFormatter should not contain parts that are specific for the copied class. For example, may you spot and remove the part that has been copied from the LinkFormatter class, which should be removed?

    $elements['url_only'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('URL only'),
      '#default_value' => $this->getSetting('url_only'),
      '#access' => $this->getPluginId() == 'smartlink_formatter',
    ];
    $elements['url_plain'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Show URL as plain text'),
      '#default_value' => $this->getSetting('url_plain'),
      '#access' => $this->getPluginId() == 'smartlink_formatter',
      '#states' => [
        'visible' => [
          ':input[name*="url_only"]' => ['checked' => TRUE],
        ],
      ],
    ];

        if (!empty($item->_attributes)) {
          // Piggyback on the metadata attributes, which will be placed in the
          // field template wrapper, and set the URL value in a content
          // attribute.
          // @todo Does RDF need a URL rather than an internal URI here?
          // @see \Drupal\Tests\rdf\Kernel\Field\LinkFieldRdfaTest.
          $content = str_replace('internal:/', '', $item->uri);
          $item->_attributes += ['content' => $content];
        }

The following code is not catching the thrown exception, contrary to the LinkFormatter class.

$url = $item->getUrl() ?: Url::fromRoute('<none>');

It would be better to catch it and change the code as per 📌 Add logging to LinkFormatter and LinkWidget when the URL is invalid Active , which is referenced from the original code.

        try {
            $url = $item->getUrl();
        } catch (\InvalidArgumentException $e) {
            // @todo Add logging here in https://www.drupal.org/project/drupal/issues/3348020
            $url = Url::fromRoute('<none>');
        }

Some of the translatable strings should be changed, but that is not blocking this application. For example:

• The hyphen in Show URL only as plain-text is not correct in English
• Instead of Add Class in link @link_class, it is preferable to use Add @link_class to links (In English, it is add something to, not add something in.)

Hello @apaderno

I have applied the all required changes as you mentioned in above comment.
Please review the module.
Thank you

    $elements = parent::settingsForm($form, $form_state);

    $elements['url_only']['#access'] = $this->getPluginId() == 'smartlink_formatter';
    $elements['url_plain']['#access'] = $this->getPluginId() == 'smartlink_formatter';
    $elements['link_class'] = [
      '#type' => 'textfield',
      '#title' => $this->t('Link class'),
      '#default_value' => $this->getSetting('link_class'),
      '#required' => FALSE,
      '#states' => [
        'invisible' => [
          ':input[name*="url_plain"]' => ['checked' => TRUE],
        ],
      ],
    ];
    $elements['rel'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Add rel="nofollow" to links'),
      '#return_value' => 'nofollow',
      '#default_value' => $this->getSetting('rel'),
      '#states' => [
        'invisible' => [
          ':input[name*="url_plain"]' => ['checked' => TRUE],
        ],
      ],
    ];

    if ($this->getPluginId() == 'smartlink_formatter' && !empty($settings['url_only'])) {
      if (!empty($settings['url_plain'])) {
        $summary[] = $this->t('Show URL only as plain text');
      }
      else {
        $summary[] = $this->t('Show URL only');
      }
    }

The question in the previous comment has not been answered, nor has the code been correctly changed. Can you spot the lines that have been copied from the LinkFormatter class, which should not be used in this class, and explain why those lines are not correct?

$url = $item->getUrl() ?: Url::fromRoute('<none>');

That line is not yet catching the thrown exception. The code should also be changed as per 📌 Add logging to LinkFormatter and LinkWidget when the URL is invalid Active .

Hello @apaderno

I've eliminated the duplicated code from the link formatter. Instead, I've modified the field's access and addressed the states included by the link formatter. Additionally, I've incorporated the necessary exception handling as per https://www.drupal.org/project/drupal/issues/3348020 📌 Add logging to LinkFormatter and LinkWidget when the URL is invalid Active

Thank you

    $elements['url_only']['#access'] = $this->getPluginId() == 'smartlink_formatter';
    $elements['url_plain']['#access'] = $this->getPluginId() == 'smartlink_formatter';
    $elements['rel']['#states'] = [
      'invisible' => [
        ':input[name*="url_plain"]' => ['checked' => TRUE],
      ]
    ];
    $elements['target']['#states'] = [
      'invisible' => [
        ':input[name*="url_plain"]' => ['checked' => TRUE],
      ]
    ];
    $elements['noopener'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Add rel="noopener" to links'),
      '#return_value' => 'noopener',
      '#default_value' => $this->getSetting('noopener'),
      '#states' => [
        'invisible' => [
          ':input[name*="url_plain"]' => ['checked' => TRUE],
        ],
      ],
    ];

    if ($this->getPluginId() == 'smartlink_formatter' && !empty($settings['url_only'])) {
      if (!empty($settings['url_plain'])) {
        $summary[] = $this->t('Show URL only as text');
      }
      else {
        $summary[] = $this->t('Show URL only');
      }
    }

The code I quoted in my previous comment still contains lines that are present because the Drupal core class has similar lines.
Notice that in my previous comment I asked a question for which I am waiting an answer: Can you spot the lines that have been copied from the LinkFormatter class, although they have been changed, which should not be used in this class, and explain why those lines are not correct?

    try {
      $url = $item->getUrl();
    }
    catch (\InvalidArgumentException $e) {
      // @todo Add logging here in https://www.drupal.org/project/drupal/issues/3348020
      $url = Url::fromRoute('<none>');
    }

The comment is linking to a Drupal core issue, which will not change this module's code. You can open an issue for your project, and reference that issue.

Hello @apaderno

1. I have removed the duplicate code which is already exists in base class linkformatter.
2. I have created the issue and refer in module that issue.

Thank you

The changes are not correct.
Instead of changing the code, please answer this question: Can you spot the lines that have been copied from the LinkFormatter class, which should not be used in this class, and explain why those lines are not correct?

The reason I am asking that question is simple: Most of the code used by the project comes from a Drupal core class. The code you wrote is not sufficient for these applications, but I could accept it if you were able to catch which lines should not be present in the code you wrote and explain why.
Copying code from another module or Drupal code and not being able to adapt it is not what we expect in these applications.

Hello @apaderno

I totally agree with you. Actually, I want to create one formatter which have all attribute support for tag that's why i extended the core class and update the another attributes for link in smartlink formatter.

Thank you

@apaderno
I hope you got my concern why i used the linkformatter code. Because i want to extend link formatter functionality with more attributes.

Please confirm what is pending to do in this module.

Please let me know if any more changes required to do
Thank you

I have already said what is wrong. I also asked a question which was never answered, and I also asked to please not change the code but simply answer the question.

I apologies, now i have answered your question.
I just want to build extended version of linkformatter with all attributes.
If i was trying to fields from parent classes those are not working due to condition which is in core getplugin == 'link'

That's why i copied those fields and changed getplugin as per smartlink requirement.

Thank you

The question I asked is the following one.

May you point out which of the following lines, which has been copied from the Drupal core class and modified, should not be used by this module?

  /**
   * {@inheritdoc}
   */
  public function settingsForm(array $form, FormStateInterface $form_state) {
    $elements = parent::settingsForm($form, $form_state);

    $elements['url_only']['#access'] = $this->getPluginId() == 'smartlink_formatter';
    $elements['url_plain']['#access'] = $this->getPluginId() == 'smartlink_formatter';
    $elements['link_class'] = [
      '#type' => 'textfield',
      '#title' => $this->t('Link class'),
      '#default_value' => $this->getSetting('link_class'),
      '#required' => FALSE,
      '#states' => [
        'invisible' => [
          ':input[name*="url_plain"]' => ['checked' => TRUE],
        ],
      ],
    ];
    $elements['rel'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Add rel="nofollow" to links'),
      '#return_value' => 'nofollow',
      '#default_value' => $this->getSetting('rel'),
      '#states' => [
        'invisible' => [
          ':input[name*="url_plain"]' => ['checked' => TRUE],
        ],
      ],
    ];
    $elements['noopener'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Add rel="noopener" to links'),
      '#return_value' => 'noopener',
      '#default_value' => $this->getSetting('noopener'),
      '#states' => [
        'invisible' => [
          ':input[name*="url_plain"]' => ['checked' => TRUE],
        ],
      ],
    ];
    $elements['noreferrer'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Add rel="noreferrer" to links'),
      '#return_value' => 'noreferrer',
      '#default_value' => $this->getSetting('noreferrer'),
      '#states' => [
        'invisible' => [
          ':input[name*="url_plain"]' => ['checked' => TRUE],
        ],
      ],
    ];
    $elements['target'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Open link in new window'),
      '#return_value' => '_blank',
      '#default_value' => $this->getSetting('target'),
      '#states' => [
        'invisible' => [
          ':input[name*="url_plain"]' => ['checked' => TRUE],
        ],
      ],
    ];

    return $elements;
  }

Please, do not change code. Just answer the question in a comment.

Hint: If you compare the code shown in this comment with the code shown in comment #15 → , you should easily find which lines are not correct.