Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a min length of 3 and per_page to 20 to avoid infinite query #23841

Merged

Conversation

mgrenierfarmmedia
Copy link
Contributor

Description

Edited the post_tag block, for editor, with a minLength of 3 since it was the classic editor base length search before. I also Limited search to 20 per_page. The old -1 was triggering many AJAX request on a simple search, for nothing since we have a MAX_TERMS_SUGGESTIONS of 20. Per consequence, with a large table of terms (We're talking about >> 2000 terms) we might have been hogging resources on the server for nothing.

How has this been tested?

  • I tested the bug still being present in current version
  • I tested the 2 possible bound (1 and 2 char) and seen no autocomplete and no XHR request from my debugger
  • I tested the lower bound (3 char) and autocomplete triggers at that moment
  • I tested 3 values in bound range (4-7-10)
  • I Tested the number per_page to see if MORE page were being loaded and no further XHR are created after the first load. On all working bound (3-4-7-10) tested

Types of changes

Bug fix. Fixing issue #20734, my own. Also fixing some support request

Change is really small.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • [!] I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

…re search for nothing. And minLenght 3 was the classic editor base lenght search before. Limited searc hto 20 per_page. The old -1 was triggering many XHR request on a simple search, for nothing since we have a MAX_TERMS_SUGGESTIONS of 20.
@talldan talldan added [Type] Bug An existing feature does not function as intended First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository labels Jul 10, 2020
@talldan talldan linked an issue Jul 10, 2020 that may be closed by this pull request
Comment on lines 225 to 227
if ( search.length >= 3 ) {
this.searchRequest = this.fetchTerms({search});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@draganescu BTW, since you are the main one who answer:
This part could be removed. It was just here to keep the OLD way, but searching with 1 letter with a peR_page of 20 and a max_suggestion of 20 doesn't create anymore XML request. It's just useless call (IMHO) since no one in UI will select a tags after 1 letter type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of FlatTermSelector that is reused by other components, so keeping the length check here seems okay to me.

@mgrenierfarmmedia
Copy link
Contributor Author

mgrenierfarmmedia commented Aug 28, 2020

So, can someone merge this? it seems pretty much done and ready

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mgrenierfarmmedia, thanks for your work here! Just fix your linting issues and you'll be good to go 👍

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you and congrats on your first PR in GB @mgrenierfarmmedia - also kudos for finding this issue 💯

@ntsekouras ntsekouras merged commit fb20b4f into WordPress:master Aug 31, 2020
@github-actions
Copy link

Congratulations on your first merged pull request, @mgrenierfarmmedia! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 8.9 milestone Aug 31, 2020
@mgrenierfarmmedia
Copy link
Contributor Author

Hi @ntsekouras and @draganescu,
Thanks for both of your help.

Also, thanks for fixing the lint error there @ntsekouras, it was firing for no reason and was successful on my computer. I do not know what was the problem. Probably environment setup...

Anyway, do any of you knows when this will be release?

Best regards,
Martin

@ntsekouras
Copy link
Contributor

Hey @mgrenierfarmmedia! It will be included in GB 8.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Large terms database generating too many WP API request on search
4 participants