[1.1.x] Unique Alias Checker

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.

Fix PHPCS issues.

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

FILE: unique_alias_checker/src/Form/UniqueAliasCheckerSettingsForm.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 52 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------


FILE: unique_alias_checker/src/UniqueAliasChecker.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 15 | ERROR | Missing short description in doc comment
--------------------------------------------------------------------------------


FILE: unique_alias_checker/unique_alias_checker.info.yml
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 7 | WARNING | All dependencies must be prefixed with the project name, for example "drupal:"
--------------------------------------------------------------------------------


FILE: unique_alias_checker/unique_alias_checker.module
--------------------------------------------------------------------------------
FOUND 20 ERRORS AND 3 WARNINGS AFFECTING 19 LINES
--------------------------------------------------------------------------------
  1 | ERROR   | [x] The PHP open tag must be followed by exactly one blank line
  4 | ERROR   | [x] Doc comment short description must end with a full stop
  7 | WARNING | [x] Unused use statement
  7 | ERROR   | [x] Line indented incorrectly; expected 0 spaces, found 1
  8 | WARNING | [x] Unused use statement
  8 | ERROR   | [x] Line indented incorrectly; expected 0 spaces, found 1
  9 | WARNING | [x] Unused use statement
  9 | ERROR   | [x] Line indented incorrectly; expected 0 spaces, found 1
 11 | ERROR   | [x] Line indented incorrectly; expected 0 spaces, found 1
 11 | ERROR   | [x] Missing function doc comment
 12 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 3
 14 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 5
 15 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 3
 16 | ERROR   | [x] Line indented incorrectly; expected 0 spaces, found 1
 18 | ERROR   | [x] Line indented incorrectly; expected 0 spaces, found 1
 19 | ERROR   | [x] Expected 2 space(s) before asterisk; 1 found
 20 | ERROR   | [x] Expected 2 space(s) before asterisk; 1 found
 21 | ERROR   | [x] Expected 2 space(s) before asterisk; 1 found
 22 | ERROR   | [x] Expected 2 space(s) before asterisk; 1 found
 23 | ERROR   | [x] Expected 2 space(s) before asterisk; 1 found
 24 | ERROR   | [x] Expected 2 space(s) before asterisk; 1 found
 25 | ERROR   | [x] Expected 2 space(s) before asterisk; 1 found
 35 | ERROR   | [x] Expected newline after closing brace
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 23 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

1. Remove the LICENSE.txt file. The LICENSE file isn't necessary. It will be added automatically by the packaging script.

2. Replace README.txt with README.md file. Please take a moment to make your README.md follow the guidelines.

@mdranove,

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.

Thanks

It is a great idea to notice content managers about this behaviour.

The following are not actually a security but general advices:

1. You can simply use MODULENAME_form_node_form_alter() here
2. I'm pretty sure that entity can be retrived with $form_state->getFormObject()->getEntity() here
3. If you use setErrorByName() method you should provide the actual form element key for the first parameter
4. In case when $original_alias == $alias your method returns nothing (void). You might consider the approach when your method ends with return $original_alias != $alias; so you won't need to use isset() in the validation function
5. I belive you would like to change the class description :)

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

No: Does not follow the licensing requirements.

Create composer.json and add a correct license, e.g. GPL-2.0-or-later

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

phpcs
=====

FILE: ...aunchpad/projects/phpunit/docroot/modules/contrib/unique_alias_checker/src/Form/UniqueAliasCheckerSettingsForm.php
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
 52 | WARNING | Only string literals should be passed to t() where possible
---------------------------------------------------------------------------


FILE: ...hock/.dropsolid_launchpad/projects/phpunit/docroot/modules/contrib/unique_alias_checker/src/UniqueAliasChecker.php
---------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
---------------------------------------------------------------------------
 71 | ERROR | [x] Expected 1 blank line after function; 0 found
 72 | ERROR | [x] The closing brace for the class must have an empty line before it
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: ...k/.dropsolid_launchpad/projects/phpunit/docroot/modules/contrib/unique_alias_checker/unique_alias_checker.info.yml
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 8 | ERROR | [x] Expected 1 newline at end of file; 0 found
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

phpstan
=======

---------------------------------------------------------------------------
  Line   UniqueAliasChecker.php                                                                                                                                                              
---------------------------------------------------------------------------
  22     PHPDoc tag @param for parameter $entity with type Drupal\unique_alias_checker\Drupal\Core\Entity\EntityInterface is not subtype of native type Drupal\Core\Entity\EntityInterface.  
  22     Parameter $entity of method Drupal\unique_alias_checker\UniqueAliasChecker::checkAlias() has invalid type Drupal\unique_alias_checker\Drupal\Core\Entity\EntityInterface.           
---------------------------------------------------------------------------


 [ERROR] Found 2 errors

Extra comments/proposals:

1. (*)UniqueAliasChecker.php:52 '#default_value' => $this->t($default_error_msg), - this is an incorrect implementation here. You shouldn't translate the default value and config setting with t(). You have to implement a translatable config instead. For that, you have to create a correct translatable schema. Afterward, you will be able to translate this config setting via Configuration Translation module
2. UniqueAliasChecker.php:22 - must be @param \Drupal\Core\Entity\EntityInterface $entity (missed \ before Drupal)

This review uses the Project Application Review Template.

Extra things

1. Service class doesn't implement an interface which is not good practice.
2. Validation was provided only for nodes, but other entity types can use aliases too. Simple example: taxonomy terms.

@mdranove As a side note, it is not necessary to create a new tag, a new release, nor a new branch every time that what reported has been changed. Commit the changes in the same branch without creating tags or releases.

A correct branch ends with the literal .x. (x is not a placeholder for a number.)

Please commit the last changes in the 1.1.x branch. The other branches, like 1.1.4. do not have a correct name.

As side note, please check Git on your computer is set to use the same email associated to your drupal.org account, or the commits are not associated to your account.

class UniqueAliasCheckerSettingsForm extends ConfigFormBase implements ContainerInjectionInterface {

That interface is already implemented by the parent class. There is no need to repeat implements ContainerInjectionInterface.

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.