[D8]youtube play channel

Git clone command for the sandbox is missing in the issue summary, please add it.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Please update the issue title by prefixing the version of Drupal this module is written for e.g [D7], [D8], [D7][D8].

Missing multiple points in the Project Application Checklist, please see https://www.drupal.org/node/1587704

Manual Review

Individual user account

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

No duplication

Pending review

Master Branch

[No: Does not follow ] the guidelines for master branch.

Licensing

[Yes: Follows ] the licensing requirements.

3rd party assets/code

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

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

[Yes: Meets the security requirements.

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. (*)Use configure option in info.yml
2. (+) Move the options like youtube id, width, height to block form, in this way blocks can be reused

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.

Automated Review

[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.

Master Branch

[No: Does not follow] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

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

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

[Yes: Meets the security requirements.]

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. (*) Major finding, needs work
2. (+) Release blocker
3. update the par review URl correctly currently its showing 404
4. Implements hook_help() https://api.drupal.org/api/drupal/core!modules!help!help.api.php/functio...
5. Specify what is the difference beteween this module and your https://www.drupal.org/project/youtubechannel in your description Similar projects and how they are different
6. Moving from a master to a major version branch https://www.drupal.org/node/1127732 check this URL
7. Readme.txt Add Table of contents in the header
8. Add a image snapshot of how your module looks in Drupal 8 for reference

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.

I am uploading few typo and tweaks a s patch

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2693479

Project 2: https://www.drupal.org/node/2682735

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

@jabastin.ameex You need to delete the master branch according to the instructions on this page: https://www.drupal.org/node/1127732

That alone is not an application blocker, anything else that you found or should this be RTBC instead?

Yes, the code looks good, so marking RTBC.

It would be great if the YouTube channel settings could be defined in the block configuration, so a site could display multiple channels, instead of just one. I think that's what @nishkris was alluding to in comment #8.

Git errors:

• There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732

Review of the 8.x-1.x branch (commit 866295f):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. What is the use case of the module? I can just create an HTML block and paste the embed code for a youtube playlist? What are the benefits of the module compared to just using an embed code? Please add that to the project page.
2. youtube_play_channel.routing.yml: why is there a /test path? This could easily clash with other modules or site functionality that wants to have different stuff on the /test path. I think you should rather use something like /youtube_play_channel_test
3. YoutubePlayController: do not use \Drupal in classes, inject the request service instead. See https://www.drupal.org/node/2133171
4. youtube_play_channel_theme(): why do you call YoutubePlayController here? You should only provide the names of all the variables here, so it does not make sense to call a controller. See the https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!theme.api.... example.
5. templates/YoutubeBlock.html.twig: indentation error for one {% else %} statement, make sure it is on the same level as the if statement.
6. templates/YoutubeBlock.html.twig: "Could not fetch videos from youtube play.": all user facing text must run through the translation system. See https://www.drupal.org/developing/api/8/localization
7. YoutubePlayController: do not silently eat exceptions and then continue as usual. This will lead to fatal errors as $response might not be set at all. Provide a proper error response in case something goes wrong.
8. YoutubeBlock: this is where you should build the actual content, not in YoutubePlayController. Move your code to the build method of this plugin, then I think you don't need the controller at all?
9. CustomForm: don't use t() in classes, use $this->t() instead.
10. CustomForm::validateForm(): a form validation handler must not save stuff, this must be done in a submission handler. The validation handler should only verify the submitted data, but not save it yet.
11. CustomForm::validateForm(): why do you have this long "$this->configFactory()->getEditable('youtube_play_channel.settings')->set('youtube_play_channel_api', $api_key)->save();" calls when this can be just something like $this->config('forum.settings')->set('topics.hot_threshold', $form_state->getValue('forum_hot_topic')) as for example in ForumSettingsForm in core?

Because of the number of wrong API usages I think this needs some work. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Only the issue of Git command/master branch. Set the default branch beacuse there is still a master branch.
Set the default branch and delete the master branch.

Steps to set default branch:-
1) Edit your project
2) Click the "Default branch" tab
3) Select the desired branch
4) Click Save

Step to Delete the master branch:-
1) git checkout 7.x-1.x
2) git branch -D master
3) git push origin master

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.