[D9] Media Video Micromodal

Thank you for applying! I added the Git instructions for non-maintainer users and the PAreview checklist link. Reviewers will check the project and post comments to list what should be changed.

If you haven't done it, yet, please check the PAreview report and fix what needs to be fixed. There could be some false positives; verify that what reported is correct, before making any change.

Please go through the PAreview checklist as it shows few issues are still there. Let me know if I can help.
http://pareview.net/r/227

Thanks,
Rajeshreeputra

A review needs to be more than See this link for a list of issues to be resolved. That's why I didn't change status when I wrote my previous comment.

I changed the issue priority as described on Review process for security advisory coverage: What to expect / Application Review Timelines.

To reviewers: Please change back the priority to Normal after reviewing the project.

• What follows is a quick review of the project; it doesn't mean to be complete
• For every point, the review doesn't make a complete list of lines that should be fixed, but an example of what is wrong in the code
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; if a point isn't about that, it makes it clear

    if (!empty($this->getSetting('string_classes'))) {
      $summary[] = 'Additional Classes: ' . $this->getSetting('string_classes');
    }

Strings shown in the user interface needs to be translated; this also sanitize values entered from users (in this case, the settings) when rendered inside HTML markup.

@scott_earnest - You might also want to delete the 1.x branch and follow the recommended branch naming conventions - https://www.drupal.org/node/1127732. I see that you already have proper branch names. So you might want to delete the 1.x branch and that should take care of the git issue related to the branch.

1.x is a correct branch name, and it's show as release branch name in Release naming conventions / Release branches.

What is wrong is the 1.0.0 branch, as that is a tag name, not a branch name. That is not a tag name for a 1.x branch, though. The 1.0.0 branch needs to be removed.

Automated Review

Since the automated testing is not working, I ran the code through phpcs.

phpcs --standard=Drupal media_video_micromodal
FILE: media_video_micromodal/src/Plugin/Field/FieldFormatter/MicromodalFieldFormatter.php
------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------
431 | WARNING | [x] '@todo: this always returns "null", maybe b/c of this bug:' should match the format '@todo Fix
| | problem X here.'

phpcs --standard=DrupalPractice media_video_micromodal
FILE: /var/www/john/d5/test/web/modules/custom/media_video_micromodal/js/micromodal.min.js
------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------
1 | WARNING | File appears to be minified and cannot be processed
------------------------------------------------------------------------------------------

FILE: media_video_micromodal/src/Plugin/Field/FieldFormatter/MicromodalFieldFormatter.php
------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------
236 | WARNING | Media::load calls should be avoided in classes, use dependency injection instead
------------------------------------------------------------------------------------------------------------------------

FILE: media_video_micromodal/js/micromodal.min.js
------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------
1 | WARNING | File appears to be minified and cannot be processed
------------------------------------------------------------------------------------------

Since the minified file is a 3rd party library this shouldn't be an issue.

[Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should be addressed.]

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

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

There are several video modules that provide some options for modal display. I did not look closely at all of them. This module does seem to provide some additional features.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[No: Does not follow] the guidelines for 3rd party assets/code.

The license information for the 3rd party asset is missing.

README.txt/README.md

[No: Does not follow] 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

[No: List of security issues identified.] See Writing Secure Code and Translation API overview.

The t function should be used for static strings or strings that have known variables as they will be used for language translation. In #8 you would use t('Additional Classes: '). See Writing Secure Code for dealing with user entered variables that will be included in HTML

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) Security and Translation issues
2. (*) 3rd party license and README file

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

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 IRC #drupal-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 dedicated reviewers as well.