Drupal 8 module version - Roadmap

Switching to "Needs Review"

Nice to see this, just a quick eyeball review.

1. +++ b/src/Controller/AuthenticationController.php
   @@ -0,0 +1,222 @@
   +  /**
   +   * View profile page.
   +   *
   ...
   +   *   The incoming HTTP request.
   +   */
   ...
   +  /**
   +   * Login or reset a password for a user using Janrain API.
   +   *
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   *   The incoming HTTP request.
   +   */
   +  public function login(Request $request) {
   

   Nit: missing @return tag

2. +++ b/src/Controller/AuthenticationController.php
   @@ -0,0 +1,222 @@
   +        drupal_set_message($this->t('You have been successfully logged in via one-time login link.'));
   ...
   +      drupal_set_message($e->getMessage(), 'error');
   

   Deprecated, see https://www.drupal.org/node/2774931

3. +++ b/src/EventSubscriber.php
   @@ -0,0 +1,80 @@
   +      $user = User::load(\Drupal::currentUser()->id());
   

   Missing use statement use Drupal\user\Entity\User;

4. +++ b/src/ScreenLoaderManager.php
   @@ -0,0 +1,171 @@
   +    if (!file_prepare_directory($cache_directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS)) {
   

   This line is using deprecated code, use the file_system service, FileSystemInterface::CREATE_DIRECTORY and FileSystemInterface::MODIFY_PERMISSIONS

5. +++ b/src/ScreenLoaderManager.php
   @@ -0,0 +1,171 @@
   +        if (file_unmanaged_save_data($response->getBody(), $screen_destination, FILE_EXISTS_REPLACE) === FALSE) {
   

   Again, deprecated code.

6. +++ b/tests/src/Functional/JanrainCaptureTest.php
   @@ -0,0 +1,51 @@
   +  /**
   +   * The profile to use.
   +   *
   +   * @var string
   +   */
   +  protected $profile = 'janrain_capture_profile';
   

   I believe we probably should omit this no?

7. +++ b/tests/src/Functional/JanrainCaptureTest.php
   @@ -0,0 +1,51 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUp() {
   +    parent::setUp();
   +    // Reset root username.
   +    $this->rootUser->name = 'admin';
   +  }
   

   This isn't necessary I believe.

Fixed and removed the unnecessary code

OOps wrong file

Quick interdiff review:

1. +++ b/src/Controller/AuthenticationController.php
   @@ -5,6 +5,9 @@ namespace Drupal\janrain_capture\Controller;
   +use Drupal\Core\Messenger\MessengerInterface;
   +use Drupal\Core\Messenger;
   +use Drupal\Core\Messenger\Messenger;
   
   @@ -145,7 +151,7 @@ class AuthenticationController extends ControllerBase {
          }
   
   @@ -153,7 +159,7 @@ class AuthenticationController extends ControllerBase {
   -      drupal_set_message($e->getMessage(), 'error');
   

   These use statements aren't necessary.

2. +++ b/src/Controller/AuthenticationController.php
   @@ -145,7 +151,7 @@ class AuthenticationController extends ControllerBase {
   -        drupal_set_message($this->t('You have been successfully logged in via one-time login link.'));
   +        \Drupal::messenger()->addStatus($this->t('You have been successfully logged in via one-time login link.'));
   
   @@ -153,7 +159,7 @@ class AuthenticationController extends ControllerBase {
   -      drupal_set_message($e->getMessage(), 'error');
   +      \Drupal::messenger()->addError($e->getMessage());
   

   Let's use $this->messenger() instead.

3. +++ b/src/ScreenLoaderManager.php
   @@ -120,12 +121,14 @@ class ScreenLoaderManager {
   -    if (!file_prepare_directory($cache_directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS)) {
   -      $this->logger->error('Failed to create screen cache directory: @directory', [
   -        '@directory' => $cache_directory,
   -      ]);
   +    $success = \Drupal::service('file_system')
   +        ->prepareDirectory($cache_directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS);
   
   @@ -142,7 +145,10 @@ class ScreenLoaderManager {
   -        if (file_unmanaged_save_data($response->getBody(), $screen_destination, FILE_EXISTS_REPLACE) === FALSE) {
   +        $success_file_save = \Drupal::service('file_system')
   +              ->saveData($response->getBody(), $screen_destination, FILE_EXISTS_REPLACE);
   +
   +        if ($success_file_save === FALSE) {
   

   Let's inject the file_system service into janrain_capture.screen_loader_manager instead.

Should be fine now

Thanks @victoru for following up on this, looks like you included unrelated files in this (the .idea folder).

Also please attach an interdiff so we can review changes since the last patch easier.

Hi Manuel, see the latest .idea folder was removed.

Thanks @victoru for removing the .idea folder.

All points on #9 still need to be addressed.

thanks @manuel , the issue with the latest patches was that I accidentally included the previous .patch in them , can you review #14 and the interdiff is for 6_14

1. +++ b/janrain-capture-screens/edit-profile.html
   @@ -0,0 +1,88 @@
   +      </div>
   +      ¶
   +      <div class="col250">
   

   This whitespace needs removing.

2. +++ b/janrain-capture-screens/verify.html
   @@ -0,0 +1,26 @@
   +<div style="display: none" id="verifyEmail">    ¶
   +    <h2>Sorry we could not verify that email address.</h2>    ¶
   +    <p>Enter your email below and we will send you a new verification email:</p>    ¶
   +    {* #resendVerificationForm *}    ¶
   +      {* traditionalSignIn_emailAddress *}    ¶
   +      <div class="capture_footer">    ¶
   +      {* submitButton *}    ¶
   +      </div>    ¶
   +    {* /resendVerificationForm *}    ¶
   +</div>    ¶
   +<div style="display:none;" id="resendVerificationSuccess">    ¶
   +    <h2>New verification link sent.</h2>    ¶
   +    <p>Check your email for a new verification link.</p>    ¶
   +    <div class="capture_footer">    ¶
   +      <a href="./.." class="capture_btn capture_primary">Done</a>    ¶
   +    </div>    ¶
   +</div>    ¶
   +<div style="display: none" id="verifyEmailSuccess">    ¶
   +    <h2>Thanks for verifying your email address.</h2>    ¶
   +    <a href="./..">Done</a>    ¶
   

   WHOA! lots of whitespace to remove.

3. +++ b/janrain_capture-D8_initial_release-3035210-6.patch
   @@ -0,0 +1,4190 @@
   ++      </div>
   ++      ¶
   ++      <div class="col250">
   

   More whitespace.

4. +++ b/janrain_capture-D8_initial_release-3035210-6.patch
   @@ -0,0 +1,4190 @@
   ++<div style="display: none" id="verifyEmail">    ¶
   ++    <h2>Sorry we could not verify that email address.</h2>    ¶
   ++    <p>Enter your email below and we will send you a new verification email:</p>    ¶
   ++    {* #resendVerificationForm *}    ¶
   ++      {* traditionalSignIn_emailAddress *}    ¶
   ++      <div class="capture_footer">    ¶
   ++      {* submitButton *}    ¶
   ++      </div>    ¶
   ++    {* /resendVerificationForm *}    ¶
   ++</div>    ¶
   ++<div style="display:none;" id="resendVerificationSuccess">    ¶
   ++    <h2>New verification link sent.</h2>    ¶
   ++    <p>Check your email for a new verification link.</p>    ¶
   ++    <div class="capture_footer">    ¶
   ++      <a href="./.." class="capture_btn capture_primary">Done</a>    ¶
   ++    </div>    ¶
   ++</div>    ¶
   ++<div style="display: none" id="verifyEmailSuccess">    ¶
   ++    <h2>Thanks for verifying your email address.</h2>    ¶
   ++    <a href="./..">Done</a>    ¶
   

   Whitespace.

5. +++ b/janrain_capture-D8_initial_release-3035210-6.patch
   @@ -0,0 +1,4190 @@
   ++        // ToDo: Watchdog.
   

   This should be a pretty simple todo, can we do it?

6. +++ b/janrain_capture-D8_initial_release-3035210-6.patch
   @@ -0,0 +1,4190 @@
   ++    \Drupal::logger('my_module')->debug('viewProfile');
   ...
   ++      $user = User::load(\Drupal::currentUser()->id());
   ...
   ++        \Drupal::messenger()->addStatus($this->t('You have been successfully logged in via one-time login link.'));
   ...
   ++      $module_handler = \Drupal::moduleHandler();
   ...
   ++      \Drupal::messenger()->addError($e->getMessage());
   ...
   ++      $user = User::load(\Drupal::currentUser()->id());
   ...
   ++    $success = \Drupal::service('file_system')
   ...
   ++        $success_file_save = \Drupal::service('file_system')
   
   +++ b/src/Controller/AuthenticationController.php
   @@ -0,0 +1,225 @@
   +    \Drupal::logger('my_module')->debug('viewProfile');
   ...
   +      $user = User::load(\Drupal::currentUser()->id());
   ...
   +      $module_handler = \Drupal::moduleHandler();
   
   +++ b/src/EventSubscriber.php
   @@ -0,0 +1,81 @@
   +      $user = User::load(\Drupal::currentUser()->id());
   

   Can these services be injected rather than using `\Drupal::`?

   Also it seems like a couple of uses of logger() are logging as "my_module" as if it's been pasted from example code, these need to be replaced by the correct janrain module name.

7. +++ b/src/ScreenLoaderManager.php
   @@ -0,0 +1,177 @@
   +    $success = \Drupal::service('file_system')
   ...
   +        $success_file_save = \Drupal::service('file_system')
   

   Can this service be injected?

@th_tushar - Can you enable testing on the 8.x branch so we can run testbot on this patch?

Looks like tests are failing on PHP 5, which is fine, I guess. Everyone should be running PHP 7 these days.

On PHP 7 I see JanrainCaptureTest::testJanrainCapture is failing as it returns a 404 when expecting a 200.

Then many coding standards issue, mostly css, which as a non-frontend person I can forgive 🤪

However there are some php issues too:

src/Controller/AuthenticationController.php
  128	Line exceeds 80 characters; contains 89 characters
  159	Line indented incorrectly; expected 6 spaces, found 8
src/ScreenLoaderManager.php
  125	Object operator not indented correctly; expected 6 spaces but found 8
  129	Array indentation error, expected 8 spaces but found 10
  130	Array closing indentation error, expected 6 spaces but found 7
  131	Line indented incorrectly; expected 6 spaces, found 7
  149	Object operator not indented correctly; expected 10 spaces but found 14

+++ b/src/Controller/AuthenticationController.php
@@ -0,0 +1,225 @@
+    \Drupal::logger('my_module')->debug('viewProfile');

This to me looks like it needs to be removed.

Fixed all the spacing issues and removed the logger

Took out the unit test and resolved comments related to file_system.

Is that test not supposed to pass?

I added that forgot password check, @timmilwood , the issue with the failed phpunit was because the module itself wasnt enabled during the test, 'public static $modules = ['janrain_capture'];' fixed it , I also resolved some notices with this latest patch.

janrain_capture_mapping/janrain_capture_mapping.module
10	Unused use statement

Looks like that might be a false report.

Lets get this released.