[D8] StoryChief

Hi @gregory-claeyssens, this looks good at first glance. I'm attaching the feedback from pareview.sh. Most of the items should be easy to fix (e.g. single README file, etc.). At first I thought all the "Doc comment short description must start with a capital letter" errors tied to {@inheritDoc} annotations were false positives...but it looks like all you have to do is rename {@inheritDoc} to {@inheritdoc} to satisfy that standard (cf).

Abridged pareview.sh output:

• There should be only one README file, either README.md or README.txt.

FILE: ...t/drupal/contrib/storychief/repo/src/Form/StoryChiefSettingsForm.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 23 WARNINGS AFFECTING 22 LINES
--------------------------------------------------------------------------
  78 | WARNING | t() calls should be avoided in classes, use dependency
     |         | injection and $this->t() instead


FILE: ...epo/src/EventSubscribers/StoryChiefRemoteCallTestEventSubscriber.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
 48 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
--------------------------------------------------------------------------

FILE: ...rupal/contrib/storychief/repo/config/install/storychief.settings.yml
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
 2 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: ...Users/ndewhurst/Development/drupal/contrib/storychief/repo/README.md
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
--------------------------------------------------------------------------
  4 | WARNING | Line exceeds 80 characters; contains 104 characters
--------------------------------------------------------------------------


FILE: ...t/drupal/contrib/storychief/repo/src/Form/StoryChiefSettingsForm.php
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
--------------------------------------------------------------------------
 30 | ERROR | [ ] Doc comment short description must start with a capital
    |       |     letter
 30 | ERROR | [x] Doc comment short description must end with a full stop
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Automated Review

See #3

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.

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. The README files look pretty good and appear to follow the README Template. Changes to make: (a) include only one README file in the repo (.txt or .md), (b) consider updating the project page and/or linked documentation to match the README contents.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Appears to meet the security requirements. User input is minimal. Database interactions use abstraction layer. Anonymous users/bots can hit the StoryChief webhook endpoint with semi-valid data. DDOS issues related to that could be mitigated by this module or by external means, but this seems out of scope regardless. It looks as though StoryChiefWebhookController is handling webhook requests in a sensible manner.

Coding style & Drupal API usage

See Automated review in previous comment

This review uses the Project Application Review Template.

@gregory-claeyssens cool. Automated testing looks good now. I created a trial StoryChief account to do some basic manual testing...

Feedback:

• I created a SimplyTest.me site with the StoryChief module enabled
• I defined the field mapping in Drupal and saved the basic settings
• I established the test site as a channel in StoryChief
• I created a story in StoryChief, but when attempting to publish it, the site returned a 500 error. Upon further inspection, I saw that this exception was thrown: Drupal\storychief\Exceptions\InvalidLanguageException: The language provided is not available on this website. in Drupal\storychief\Plugin\StoryChiefFieldHandler\LanguageStoryChiefFieldHandler->set() (line 35 of /var/www/html.original/modules/contrib/storychief/src/Plugin/StoryChiefFieldHandler/LanguageStoryChiefFieldHandler.php)..
• I had to look into this on my own...and was able to resolve it by adding English as a default language in my StoryChief account (to match the language of the Drupal test site). (Account > Settings > Stories > Languages > Add language). You should add this to your documentation.
• Once the integration succeeded, I confirmed that an article was created on the Drupal site, and the SEO title was set as configured in StoryChief.
• It seems that the "API key" the Drupal module uses is actually a short encryption key. This is different than the long API access tokens you can generate in StoryChief via "API > Your API Keys > + New Token." Therefore, I would recommend this module refer to the key as an "encryption key" or something of that nature, to avoid confusion (especially since it seems plausible that the module may, in the future, wish to use what StoryChief refers to as "API Keys" for other types of integration. Let me know if I'm misunderstanding the terminology, etc.

Thanks @gregory-claeyssens. Those updates look good. I confirmed that publishing a story with no languages defined in StoryChief now succeeds. A final automated code review shows no issues introduced by recent commits. As far as I'm aware, it's reasonable to now mark this RTBC.
Also, if you or someone on your team have a moment, I'm looking for someone to review this application, which comes with a sample account connection so it should be easy to test :)

Not sure why you implement your own _is_storychief_hmac_valid access system. You can use the _custom_access route requirement and a method on your class, then this is simpler. See https://www.drupal.org/docs/8/api/routing-system/access-checking-on-rout...

Otherwise looks good to me.

Thanks for your contribution, Gregory!

I updated your account so you can opt into security advisory coverage now.

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 Slack or 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.