[1.0.x] Fuzzy Entity Reference

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

Please read Review process for security advisory coverage: What to expect for more details and Security advisory coverage application checklist to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

To reviewers: Please read How to review security advisory coverage applications, What to cover in an application review, and Drupal.org security advisory coverage application workflow.

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

Hello @mohit_aghera,

I have reviewed the code and it look fine to me.

Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.

Thanks

hi @mohit_aghera

• I would set requirements like this core_version_requirement: ^8.8 || ^9 || ^10 and not core_version_requirement: ^8 || ^9 || ^10 because it has 10 too.
• Remove core: 8.x from info file
• Change this line to this getReferenceableEntities(bool $match = NULL, string $match_operator = 'CONTAINS', int $limit = 10) and test it.
• As a suggestion, add the PHP version requirements in the composer.json file.

The rest seems to be ok.

Regards,
Andrei

Hi @mohit_aghera again

I see. It's an extend probably. No worries then. For me, this is more than ok!

Regards,
Andrei

General remark:
This appreach loads all the possible entities that can possibly be referenced in order to build the source of the fuzzy search.
This can be an issues for sites with a large number of entities - this should be explained on the project detail page.
Even for small/medium data sets, IMHO a major performance imporovement would be to cache that data and only invalidate on ENTITY_TYPE_list:BUNDLE cache tags.

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

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

List of identified issues in no particular order

1. Just a recommendation: split larger method into smaller chunks for readability

This review uses the Project Application Review Template.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

Since the module is compatible with Drupal 8.8.x and Drupal 9.x, it cannot use features implemented in PHP 8.1 without declaring the minimum required PHP version. Either that or the code is changed to be compatible with the PHP version required by Drupal 8.8.x.

src/Plugin/Field/FieldWidget/FuzzyAutocompleteWidget.php

  /**
   * {@inheritdoc}
   */
  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, array $third_party_settings, AccountProxyInterface $current_user) {
    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $third_party_settings);
    $this->currentUser = $current_user;
  }

{@inheritdoc} is not used for class constructors.

I am referring to the documentation comment before the class constructor.

  /**
   * {@inheritdoc}
   */
  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, array $third_party_settings, AccountProxyInterface $current_user) {
    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $third_party_settings);
    $this->currentUser = $current_user;
  }

{@inheritdoc} is not used for class constructors. Their documentation comments need to document the parameters the constructor receives, but not the return value.

@mohit_aghera I'm proposing to cache the list of entity labels used to run the search on (and not caching the search results themselves)

https://git.drupalcode.org/project/fuzzy_entity_reference/-/blob/1.0.x/s..., lines 57-82, aka. the $matches variable calculation could be cached until new labels are added / existing ones are updated

Thank you for your contribution! I am going to update your account.

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 Slack #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 reviewers.