[1.0.x] Image Compare Viewer

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.

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

FILE: contrib/icm/icm.info.yml
-------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 2 LINES
-------------------------------------------------------------------------------------------------------------
 9 | WARNING | All dependencies must be prefixed with the project name, for example "drupal:"
-------------------------------------------------------------------------------------------------------------


FILE: contrib/icm/src/Plugin/Field/FieldFormatter/IcmFieldFormatter.php
----------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 8 WARNINGS AFFECTING 9 LINES
----------------------------------------------------------------------------------------------------------------------------------------------
  13 | ERROR   | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Entity\EntityTypeManagerInterface.
 134 | 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
 151 | WARNING | [ ] t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 155 | WARNING | [ ] t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 161 | WARNING | [ ] t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 178 | WARNING | [ ] t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 239 | WARNING | [ ] Unused variable $element_name.
 242 | WARNING | [ ] Unused variable $key.
----------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------------------------------------------------

Some notes:

1. The info file states the dependency drupal:jquery_ui, but this module is not part of core since version 8.8. Also, is this dependency really necessary? If so, then it should also be declared in icm.libraries.yml.
2. The IcmFieldFormatter announces the implementation of the interface ContainerFactoryPluginInterface which is not necessary because the FormatterBase already implements it.
3. Consider using the factory injection pattern to avoid dependency on the parent class. See Using a Factory to Create Services.
4. The includes in IcmFieldFormatter should be sorted which is a coding standard in Drupal 10.
5. The comment for the aliasManager property in IcmFieldFormatter should be cleaned up. Also, this injection is probably not necessary. Just use Url::fromRoute('entity.node.canonical', ['node' => $file['nid'][0]['value']). Should be careful here when the option "content" is selected because the entity may not be a node. Then the nid array key may not exist.
6. In IcmFieldFormatter::settingsForm() the description for effects can be adjusted to "Should the image slider be vertical or horizontal?".
7. In IcmFieldFormatter::viewElements() the $file variable can be initiated later, because there are earlier returns following before it is used.
8. In IcmFieldFormatter::viewElements() you should check if $items implement EntityReferenceFieldItemListInterface before calling getEntitiesToView().
9. Also, getEntitiesToView() returns items of EntityInterface. Might be good to check or typehint that these implement the FileInterface before calling getFileUri() and createFileUrl().
10. Why is the cache max-age set to zero for the field formatter?
11. Use consistent quotes to wrap strings. There is a mixture of "..." and '...'. Probably best to go with '...'.

Actually, the jQuery UI module has never been part of Drupal core.
Since its 8.x-1.6 release sets core_version_requirement to ^9.2 || ^10, this module should use a similar value.

Thanks for clarifying. If the dependency is necessary then it should be declared as jquery_ui:jquery_ui.

FILE: icm.info.yml

core_version_requirement: ^8 || ^9 || ^10

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

Some more notes:

1. This is clearer now. Please change the variable name here (L229). $node = $items->getEntity() gets the entity this field belongs to. Later, after checking if this indeed is of NodeInterface, you can use $node->id() when resolving the Url.
2. Here (L217) you are loading the image style again, although you already have the image style.
3. Here (L213) you are using the variable name $images, but apparently these are files - at least you are using the FileInterface.
4. Here (L224) you can use $file->id() in case these are of FileInterface.
5. You can combine the two early returns in viewElements().
6. Use consistent quotes to wrap strings. There is a mixture of "..." and '...'. Probably best to go with '...'.

Thank you for your contribution!
I updated your account so you can now opt into security advisory coverage for any project you created and every project you will create.

These are some recommended readings to help you with maintainership:

• Dries' post on Responsible maintainers
• Maintainership
• Git version control system
• Issue procedures and etiquette
• Maintaining and responding to issues for a project
• Release naming conventions.

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

Thank you 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 also the dedicated reviewers as well.