[D8] Theater

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxcahodk2647734git

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.

Hi c.hoyer,

Automated Review

There are some blocking issues found during the repeated automated review:
FILE: ...l-7-pareview/pareview_temp/src/Plugin/Field/FieldType/TheaterItem.php
---------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
---------------------------------------------------------------------------
41 | ERROR | [x] Array indentation error, expected 10 spaces but found 12
44 | ERROR | [x] Line indented incorrectly; expected at least 8 spaces,
| | found 6
------------------

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
[Yes] Follows the licensing requirements.
Project Page
[No] Your project page is not very detailed, please have a look at the tips for a great project page, you may also use HTML-tags for better structure. It should at least reflect the same as the README.txt.
README.txt/README.md
[Yes] README is detailed.
[Yes] Follows the guidelines for project length and complexity.
Secure code
[Yes] Did not see any unsafe usage of javascript or php.
Coding style & Drupal API usage
[Yes] Looks good to me although I am not familiar enough with D8 yet.

Best Regards,
Helgi.

@c.hoyer this is a nice module and I think I'll use it on my site. I have done manual testing of the module and it works fine.

I would suggest the following improvements:

1. It would be nice to provide a short helpful description on the field textfield widget on what kind of input that takes. I was a bit thrown back by the validation until I went to your project page to see where I was making mistakes.
2. In TheaterItem.php the theaterValidate() method is private. In Drupal 8, private methods are not encouraged because it makes sub-classing and code reuse more difficult.
3. Also, the validation code is not easy to read and understand at a glance, will make this difficult for someone else to maintain in future.
4. The short array syntax is more preferable in D8 since it makes the code easier to read (but that depends on individual preferences anyway). There is no standard on that yet in D8.

Assigning to myself for review that may be tonight

@c.hoyer
sure, take your time, I'll pick this up after refactoring.update here when you'll be done.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: ...ules/contrib/pareview_temp/src/Plugin/Field/FieldType/TheaterItem.php
  ---------------------------------------------------------------------------
  FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
  ---------------------------------------------------------------------------
   38 | WARNING | Variable $this is undefined.
   84 | WARNING | Variable $needle is undefined.
   86 | WARNING | Variable $needle is undefined.
   93 | WARNING | Variable $actor is undefined.
   95 | WARNING | Variable $actor is undefined.
  ---------------------------------------------------------------------------
  
  Time: 198ms; Memory: 3.75Mb
  

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.

FILE: ...ules/contrib/pareview_temp/src/Plugin/Field/FieldType/TheaterItem.php
---------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 3 LINES
---------------------------------------------------------------------------
 116 | ERROR | Missing short description in doc comment
 117 | ERROR | Missing parameter comment
 117 | ERROR | Missing parameter type
 118 | ERROR | Missing parameter comment
 118 | ERROR | Missing parameter type
---------------------------------------------------------------------------


FILE: ...s/contrib/pareview_temp/tests/src/Unit/SubClasses/TheaterItemTest.php
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
 12 | WARNING | [x] Unused use statement
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

Time: 601ms; Memory: 4.75Mb

Manual Review :

1. theater_help : Usage of single quotes is recommended.No need of @see hook_help() in docs.
2. Add the configuration section in Readme.txt file
3. its better to make the directory structure for assets is css/ and js/ instead of lib/theater_init/
4. TheaterItemTest.php : why the group theater is twice in metadata of class ?

P.S. : My skills for the unit test cases are still progressing, So leaving it for another review.
Assigning to klausi to give it a final review when he'll get time.

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

manual review:

1. project page: what is the use case of this module? Why would I use it? What kind of field data would you display this way? Please describe that according to https://www.drupal.org/node/997024
2. TheaterItem.php: don't use \Drupal::typedDataManager(), use $this->getTypedDataManager() instead. Never use \Drupal in classes where possible, always use the injected services. See https://www.drupal.org/node/2133171
3. atomIsScript(): this should never be called when validating user input: "the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database" from https://www.drupal.org/node/28984 . So script tags should be allowed - they should just not be executed as script and should be escaped on output.
4. TheaterItem: I would put your constraint validation logic into a dedicated constraint class then you don't need the hack with the ComplexData callback constraint.
5. TheaterItem: why does this have an addViolation() method? Is that an override of a parent function? Why is it needed? Please add a comment.
6. TheaterFormatter: this is where the escaping of user provided values should happen.
7. TheaterItemTest: no need to use long string vlass names like 'Drupal\Core\TypedData\DataDefinitionInterface', use DataDefinitionInterface::class instead.

But that are critical application blockers, so ...

Thanks for your contribution, c.hoyer!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.