[8.x-1.x] SEO Keyword Links

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

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.

To reviewers: Please read How to review security advisory coverage applications → , What to cover in an application review → , and Drupal.org security advisory coverage application workflow → .

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

1. Fix phpcs isssues.

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

FILE: seo_keyword_links/seo_keyword_links.module
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
  87 | ERROR | [ ] Multi-line assignment not indented correctly; expected 12 spaces but found 10
 104 | ERROR | [x] Expected 1 blank line after function; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: seo_keyword_links/src/Form/SeoSettingsForm.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 14 | WARNING | The class short comment should describe what the class does and not simply repeat the class name
 46 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
--------------------------------------------------------------------------------


FILE: seo_keyword_links/src/Tests/SeoKeywordLinksTestTestCase.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------------
 20 | ERROR   | Do not disable strict config schema checking in tests. Instead ensure your module properly declares its schema for configurations.
 70 | WARNING | Possible useless method overriding detected
--------------------------------------------------------------------------------

2. FILE: seo_keyword_links.info.yml

core_version_requirement: ^8 || ^9

The Drupal Core versions before 8.7.7 do not recognize the core_version_requirement: key.

package: Custom

Modules should avoid using that value for the package.

3. FILE: seo_keyword_links.module

PHP Parse error: Unmatched '}' in seo_keyword_links.module on line 105

weird, i must have another PHPCS version, because i dont see those errors :-(
anyway i fixed all the errors i found
this is ready to review again

@leandro713,

I have reviewed the changes, and they look fine to me.

Let’s wait for other reviewers 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 doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

seo_keyword_links.module

/**
 * @file
 * Seo Keyword Links primary module file.
 *
 * Phpcbf --standard=Drupal,DrupalPractice
 *    modules/custom/keyword_link/keyword_link.module.
 *
 *
 * -- @TODO
 * - target = "_blank" option
 */

The usual short description is Hook implementations for the [module name] module. primary module does not say which module is, since there is not a primary module.

function seo_keyword_links_help(
    $route_name,
    RouteMatchInterface $route_match
) {

As per Drupal coding standards → ( Function Declarations → ) function/method declarations must be in a single line. That code is formatted following the PSR12 coding standards, which are not the coding standards Drupal uses.

          $body
            = preg_replace(
            "/\b$word\b/u",
            "<a href='" . $link . "'>" . $word . "</a>",
            $body
          );

That code is better written on a single line. It is not much readable as it is written. Drupal coding standards do not say that lines must be shorter than 81 characters, but that code must be readable.

src/Form/SeoSettingsForm.php

/**
 * Functional tests class.
 *
 * @package Drupal\seo_keyword_links\Form
 */
class SeoSettingsForm extends ConfigFormBase {

The short description is for another class.

  /**
   * Constructs a new SeoSettingsForm object.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager service.
   */

The short description does not show the class namespace.

    $_ = [];
    foreach ($form_state->getValue('node_types') as $content_type) {
      if ($content_type != "0") {
        $_[] = $content_type;
      }
    }

    $this->config('seo_keyword_links.settings')

      ->set(
            'links',
            $this->codeLinks(
                explode(
                    "\n",
                    $form_state->getValue('links')
                )
            )
        )

      ->set('node_types', $_)

      ->save();
  }

There are extraneous empty line and that code is much more readable if it is written with each method call on a single line.
I would also suggest to use a variable that is not $_, since that variable name does not follow the Drupal coding standards → .

Variables should be named using lowercase, and words should be separated either with uppercase characters (example: $lowerCamelCase) or with an underscore (example: $snake_case).

  /**
   * Inner function.
   */

That is a quite broad description that does not say what that method does, nor does it describe its parameters and their types.

      if (@$a[0] && @$a[1]) {
        $_[$a[0]] = $a[1];
      }

There is no need to use the @ operator in that case.

First of all, thank you very much for the excellent review and for the time you have dedicated to it, Alberto.
I have already corrected the issues you brought to my attention.
The 8.x-1.x branch is now ready to be reviewed again.

From my point of view, the provided settings should be non-existent examples or not provide example settings at all:

links:
  Ono:        "http://ono.es"
  Yoigo:      "http://yoigo.com"
  Vodafone:   "http://vodafone.es"
  Telefonica: "http://telefonica.net"

In the defined test.

The documentation refers only to drupal 8 but according to the info file, the module is also compatible with 9:

/**
 * Tests the Drupal 8 seo_keyword_links module functionality.
 *
 * @group seo_keyword_links
 */
class SeoKeywordLinksTestTestCase extends WebTestBase {

Also WebTestBase is deprecated in drupal 8.8 and removed in drupal 9:
https://api.drupal.org/api/drupal/core%21modules%21simpletest%21src%21We...

Alvaro, first and foremost, I would like to express my utmost gratitude for your excellent review and the time you have dedicated to it. Secondly, having taken note of your observations, I have already made some changes in order to ensure a better verdict with regards to the security review.
Personally, I strongly believed that leaving a real example in the configuration was better for the user; however, I have deleted it following your recomendations.
Additionally, I have left the old Simpletest test available for Drupal 8 users and added another one for those who use Drupal 9.

Please don't forget to review the 8.x-1.x branch.

As requested by @alvar0hurtad0, I have removed the tests.

However, this implies that without initial values in the configuration, it is impossible for me to test anything (configuration values would have to be provided to the test), making it impossible to write tests without specifying a lot of configuration, values in the node, etc.

It's quite messy and complicated :-(

1. Fix phpcs issues.

FILE: /app/web/modules/contrib/seo_keyword_links/seo_keyword_links.module
-------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
-------------------------------------------------------------------------
  8 | ERROR | [x] Doc comment long description must end with a full stop
 38 | ERROR | [x] Expected 0 spaces after opening parenthesis; 1 found
 38 | ERROR | [x] There should be no white space after an opening "("
-------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------


FILE: /app/web/modules/contrib/seo_keyword_links/src/Form/SeoSettingsForm.php
-----------------------------------------------------------------------------------------------------------------------
FOUND 20 ERRORS AFFECTING 19 LINES
-----------------------------------------------------------------------------------------------------------------------
  40 | ERROR | [ ] Missing short description in doc comment
  54 | ERROR | [x] Expected 1 space after FUNCTION keyword; 0 found
  98 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 4
  98 | ERROR | [x] Object operator not indented correctly; expected 6 spaces but found 4
  99 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 4
 100 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 4
 112 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 0
 114 | ERROR | [ ] Doc comment short description must be on a single line, further text should be a separate paragraph
 122 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 0
 123 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
 124 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
 125 | ERROR | [x] Whitespace found at end of line
 126 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
 127 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
 128 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 4
 129 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
 130 | ERROR | [x] Whitespace found at end of line
 131 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
 132 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
 133 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 0
-----------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 18 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------


FILE: /app/web/modules/contrib/seo_keyword_links/src/Tests/SeoKeywordLinksTestTestCase.9.php
----------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------
 12 | ERROR | [ ] Class name doesn't match filename; expected "class SeoKeywordLinksTestTestCase.9"
 55 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------


FILE: /app/web/modules/contrib/seo_keyword_links/src/Tests/SeoKeywordLinksTestTestCase.8.php
------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------
 12 | ERROR | Class name doesn't match filename; expected "class SeoKeywordLinksTestTestCase.8"
------------------------------------------------------------------------------------------------

2. FILE: seo_keyword_links.info.yml

most of the seo related modules are grouped using the "SEO" package.
so, You should use package: SEO instead of package: Custom

I am investigating how to pass the configuration to the module within the test. If I succeed, I will upload the tests again.

@Rajan, thank you for reviewing the code formatting. I greatly appreciate the time you have dedicated to this.
I have corrected the code formatting by running PHPCS on all my files.
The 8.x-1.x branch is now ready for review.

@leandro713, check the below comment also.

2. FILE: seo_keyword_links.info.yml

most of the seo related modules are grouped using the "SEO" package.
so, You should use package: SEO instead of package: Custom

@Rajan done; info.yml changed
branch 8.x-1.x is already ready to be reviewed again

Hi @leandro713,
you can keep the tests if you add your settings in the setup method.

Yes, Álvaro. Finally, I managed to include the module settings in the tests. Thank you for your good advice.

However, I am currently experiencing problems with my local PHPUnit installation, so I cannot check the tests at the moment.

Anyway, I don't think this issue is urgent. I'm going to leave the module without tests for now because it works quite well for me.

Once I have a bigger user base, I can always add the tests, right?

  /**
   * The entity type manager.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager service.
   */
  public function __construct(EntityTypeManagerInterface $entity_type_manager) {

The first short description is not the method description.

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the Slack → #contribute channel. So, come hang out and stay involved → .
Thank you, 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.

I thank all the reviewers.

i reopen this issue because i dont see granted the security badge on the module's webpage → https://www.drupal.org/project/seo_keyword_links →

We just assign a role to user account. The project needs to be edited by the person who created the application.

Ah, yes, sorry @apaderno
I didnt understand how this works.
So ok, fixed

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