[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 @andrei.vesterli
Fixed 1, 2 and 4th points.

Regarding point 3: I think PHP will throw error due to declaration of method compatibility.
Base class's method doesn't have typed arguments.

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

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.

@reszli
Thank you for the feedback.
I've little doubt about following:
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.
I initially tried to cache it, however, since search term can be anything, we can't cache the results based on the search term.
Can you please give me more information, it seems I am missing something here.

@apaderno
{@inheritdoc} is not used for class constructors.
It is already added here https://git.drupalcode.org/project/fuzzy_entity_reference/-/blob/1.0.x/s...
Should I describe each argument further?

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

Hi @reszli
Thanks for the feedback and guidance for the implementation.
I did refactoring so that we can cache the results for particular search term.

I've pushed the code in the branch along with test for it.
Commit https://git.drupalcode.org/project/fuzzy_entity_reference/-/commit/1df25...
(Please ignore schema changes from config, already removed in next commit)

Can you please review this and let me know how it goes?

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.

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