[D8] Photos

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 Nathaniel,

I have been reviewing your module and found that you have used depreciated db_queries in your codebase instead we should use dependency injection for database connection.

/**
 * Implements hook_photos_access().
 */
function photos_photos_access() {
  if (\Drupal::config('photos.settings')->get('photos_access_photos')) {
    $current_path = \Drupal::service('path.current')->getPath();
    $path_args = explode('/', $current_path);
    if (isset($path_args[3]) && $path_args[1] == 'photos' && $path_args[2] <> 'get' && is_numeric($path_args[3])) {
      switch ($path_args[2]) {
        case 'album':
          return [$path_args[3]];

        case 'image':
          $nid = db_query("SELECT pid FROM {photos_image} WHERE fid = :fid", [':fid' => $path_args[3]])->fetchField();
          return [$nid];
      }
    }
    if (isset($path_args[4]) && $path_args[1] == 'photos' && $path_args[2] == 'data' && is_numeric($path_args[4])) {
      return [$path_args[4]];
    }
  }
}

Hi @Nathaniel,

I have checked your module coding standards with phpcs and its looking good. However I got the following results from eslint while checking the js coding standards.

eslint photos/js/photos.dragndrop.js

   1:1    warning  Definition for rule 'no-mutable-exports' was not found         no-mutable-exports
   1:1    error    Move the invocation into the parens that contain the function  wrap-iife
   1:2    warning  Unexpected unnamed function                                    func-names
   2:3    error    'use strict' is unnecessary inside of modules                  strict
   2:3    error    Expected newline after "use strict" directive                  lines-around-directive
   4:5    error    Expected method shorthand                                      object-shorthand
   4:13   warning  Unexpected unnamed method 'attach'                             func-names
   4:23   warning  'context' is defined but never used                            no-unused-vars
   4:32   error    Block must not be padded by blank lines                        padded-blocks
   8:11   error    Expected method shorthand                                      object-shorthand
   8:17   warning  Unexpected unnamed method 'stop'                               func-names
   8:34   warning  'ui' is defined but never used                                 no-unused-vars
   9:13   error    Unexpected var, use let or const instead                       no-var
   9:17   error    Use object destructuring                                       prefer-destructuring
  10:13   error    Unexpected var, use let or const instead                       no-var
  10:17   error    Use object destructuring                                       prefer-destructuring
  11:13   error    Unexpected var, use let or const instead                       no-var
  12:13   error    Unexpected var, use let or const instead                       no-var
  13:13   error    Unexpected var, use let or const instead                       no-var
  13:27   error    Unexpected string concatenation                                prefer-template
  14:53   error    Expected a line break after this opening brace                 object-curly-newline
  14:53   error    A space is required after '{'                                  object-curly-spacing
  14:72   error    Expected property shorthand                                    object-shorthand
  14:82   error    Expected property shorthand                                    object-shorthand
  14:92   error    Expected property shorthand                                    object-shorthand
  14:102  error    Expected a line break before this closing brace                object-curly-newline
  14:102  error    A space is required before '}'                                 object-curly-spacing
  14:105  warning  Unexpected unnamed function                                    func-names
  14:105  error    Unexpected function expression                                 prefer-arrow-callback
  18:12   error    Missing trailing comma                                         comma-dangle
  23:5    error    Block must not be padded by blank lines                        padded-blocks
  23:6    error    Missing trailing comma                                         comma-dangle

eslint photos/js/photos.jeditable.js

   1:1   warning  Definition for rule 'no-mutable-exports' was not found         no-mutable-exports
   1:1   error    Move the invocation into the parens that contain the function  wrap-iife
   1:2   warning  Unexpected unnamed function                                    func-names
   2:3   error    'use strict' is unnecessary inside of modules                  strict
   2:3   error    Expected newline after "use strict" directive                  lines-around-directive
   4:5   error    Expected method shorthand                                      object-shorthand
   4:13  warning  Unexpected unnamed method 'attach'                             func-names
   4:23  warning  'context' is defined but never used                            no-unused-vars
   4:32  error    Block must not be padded by blank lines                        padded-blocks
   6:7   error    Unexpected var, use let or const instead                       no-var
   7:71  warning  Unexpected unnamed function                                    func-names
   9:10  warning  Unexpected unnamed function                                    func-names
  14:48  error    Unexpected string concatenation                                prefer-template
  15:18  error    Unexpected string concatenation                                prefer-template
  24:9   error    Expected method shorthand                                      object-shorthand
  24:19  warning  Unexpected unnamed method 'callback'                           func-names
  24:36  warning  'settings' is defined but never used                           no-unused-vars
  29:10  error    Missing trailing comma                                         comma-dangle
  30:10  warning  Unexpected unnamed function                                    func-names
  36:46  error    Unexpected string concatenation                                prefer-template
  37:18  error    Unexpected string concatenation                                prefer-template
  47:9   error    Expected method shorthand                                      object-shorthand
  47:19  warning  Unexpected unnamed method 'callback'                           func-names
  47:36  warning  'settings' is defined but never used                           no-unused-vars
  50:10  error    Missing trailing comma                                         comma-dangle
  51:10  warning  Unexpected unnamed function                                    func-names
  57:5   error    Block must not be padded by blank lines                        padded-blocks
  57:6   error    Missing trailing comma                                         comma-dangle

Please have a look.

Thanks

Hi @Nathaniel,

One thing I have noticed that you have directly used PHP super-globals($_GET) across multiple files e.g PhotosAlbum.php, PhotosAlbumController.php etc in your module.

I would like to suggest you to use Symfony Request object instead.

//replace
$limit = $_GET['limit'];
//with
$limit = \Drupal::request()->query->get('limit');

Please follow this link for more information.

Thanks

Hi @Nathaniel,

I have reviewed your module and found that in your controller files you have called a service in a static way. e.g.
1.In your PhotosController.php you have called current user service directly like

$account = \Drupal::currentUser();

2. In your PhotosAlbumController.php you have called

$nid = \Drupal::routeMatch()->getParameter('node');

Instead of this you should use dependency injection to use these services. Dependency injection is the right(better) way to call these services. You should follow this link to know more about dependency injection and you can also find the information how to use dependency injection in your module.

Hi @nathaniel,

The code looks good. Making RTBC.

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
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.