Review code in relation to D8 and Search API standards.

CommentFileSizeAuthor
#2 module-as-patch.patch16.21 KBborisson_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

beltofte created an issue. See original summary.

borisson_’s picture

FileSize
16.21 KB

Uploading module as a patch so I can comment easier without having to make an actual patch.

borisson_’s picture

Overall this is looking very nice! Adding tests is of course a good idea, but you already have another issue open for this.
I did find some other things that can be changed, most things are coding standards related.

  1. --- /dev/null
    +++ b/.gitignore
    

    This shouldn't be in the repo here. I don't see any other modules that have a gitignore.

  2. +++ b/.gitignore
    --- /dev/null
    +++ b/README.txt
    

    Some line endings in this file have spaces. Those should be removed.

  3. +++ b/README.txt
    @@ -0,0 +1,59 @@
    +It makes it possible to exclude nodes and other entities from being indexed in ¶
    +search indexed configured using Search API framework. ¶
    

    It makes it possible to exclude entities from being indexed in a Search API index.

  4. +++ b/README.txt
    --- /dev/null
    +++ b/composer.json
    

    I don't think this is needed, the drupal composer facade already creates a composer file.

  5. +++ b/search_api_exclude_entity.info.yml
    @@ -0,0 +1,8 @@
    +  - field
    +  - search_api
    

    I haven't seen any documentation about this, but as far as I know the best practice is to namespace dependencies. In this case that's drupal:field, search_api:search_api.

    This is not needed.

  6. +++ b/search_api_exclude_entity.info.yml
    @@ -0,0 +1,8 @@
    \ No newline at end of file
    

    needs eof newline

  7. +++ b/search_api_exclude_entity.module
    @@ -0,0 +1,34 @@
    \ No newline at end of file
    

    Needs eof newline.

  8. +++ b/src/Plugin/Field/FieldFormatter/SearchApiExcludeEntityFormatter.php
    @@ -0,0 +1,44 @@
    +/**
    + * @file
    + * Contains Drupal\search_api_exclude_entity\Plugin\Field\FieldFormatter\SearchApiExcludeEntityFormatter.
    + */
    

    @file docblocks are not longer needed for files that contain only one class.

  9. +++ b/src/Plugin/Field/FieldType/SearchApiExcludeEntityFieldItem.php
    @@ -0,0 +1,52 @@
    +/**
    + * @file
    + * Contains \Drupal\search_api_exclude_entity\Plugin\Field\FieldType\SearchApiExcludeEntityFieldItem.
    + */
    

    @file docblocks are not longer needed for files that contain only one class.

  10. +++ b/src/Plugin/Field/FieldWidget/SearchApiExcludeEntityFieldWidget.php
    @@ -0,0 +1,82 @@
    +/**
    + * @file
    + * Contains \Drupal\search_api_exclude_entity\Plugin\Field\FieldWidget\SearchApiExcludeEntityFieldWidget.
    + */
    

    @file docblocks are not longer needed for files that contain only one class.

  11. +++ b/src/Plugin/search_api/processor/SearchApiExcludeEntityProcessor.php
    @@ -0,0 +1,179 @@
    +/**
    + * @file
    + * Contains Drupal\search_api_exclude_entity\Plugin\search_api\processor\SearchApiExcludeEntityProcessor.
    + */
    

    @file docblocks are not longer needed for files that contain only one class.

  12. +++ b/src/Plugin/search_api/processor/SearchApiExcludeEntityProcessor.php
    @@ -0,0 +1,179 @@
    +/**
    + * @SearchApiProcessor(
    

    I think this could use a docblock title.

  13. +++ b/src/Plugin/search_api/processor/SearchApiExcludeEntityProcessor.php
    @@ -0,0 +1,179 @@
    +    $entity_manager = \Drupal::service('entity_field.manager');
    

    Proper DI here would be great, use ::create to inject the manager so we don't have to call \Drupal::service from here.

  14. +++ b/src/Plugin/search_api/processor/SearchApiExcludeEntityProcessor.php
    @@ -0,0 +1,179 @@
    +      $entity_manager = \Drupal::service('entity_field.manager');
    

    and here.

  15. +++ b/src/Plugin/search_api/processor/SearchApiExcludeEntityProcessor.php
    @@ -0,0 +1,179 @@
    +    if (isset($field_map[$entity_type][$field]['bundles'][$bundle])) {
    +      return TRUE;
    +    }
    +    else  {
    +      return FALSE;
    +    }
    

    return isset($field_map[$entity_type][$field]['bundles'][$bundle]) is the same thing.

  16. +++ b/src/Plugin/search_api/processor/SearchApiExcludeEntityProcessor.php
    @@ -0,0 +1,179 @@
    +          // We need to be sure that the field actually exists
    +          // on the bundle before getting the value to avoid
    +          // InvalidArgumentException exceptions.
    

    This comment should be reflowed to be closer to 80 cols.

  17. +++ b/src/Plugin/search_api/processor/SearchApiExcludeEntityProcessor.php
    @@ -0,0 +1,179 @@
    +            if (NULL !== $object->get($field)->getValue()[0]['value']) {
    

    The coding standards don't allow yoda if statements. The NULL should be on the other side of the !==

beltofte’s picture

Status: Active » Fixed

Thanks @borisson_ for the nice code review!

I have applied all the suggestions and committed the changes.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.