The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.It added 300ms waiting time for transliterate but for non-transliterate, it's redundancy.
(I'm almost 100% failing on form submissions because this extra 300ms)
Follow up issue:
- Needs a standard way to block form submission when waiting for ajax returns (in above case is transliterate)
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | no_delays_update_for-2668596-2.patch | 1.25 KB | borisson_ |
Comments
Comment #2
borisson_80 cols fix.
Comment #3
droplet commentedahh. Thanks. 80 cols is looks good. Can you also review this patch ?
Comment #5
droplet commentedComment #7
michielnugter commentedI reviewed the patch and it correctly removes the 300ms delay from the machine name code when no transliteration is required. I tested the patch using my created test case in https://www.drupal.org/node/1686174#comment-11742776
However the fix doesn't solve the reported issue. The machine name relies on the formUpdated event which is trigger by Core (misc/form.js). This is done using debounce with a delay of 300 ms. So earlier the delay was actually 600ms in total, now it's down to 300ms.
Because there is still a delay as long as formUpdated is used the issue is not fixed and still needs work. The only solution is not using the formUpdated event.
Also, ideally the issue should extend on the testcase posted in the mentioned issue and add a check that the delay is not present when entering a string that shouldn't need transliteration.
Comment #8
michielnugter commentedComment #9
droplet commented@michielnugter,
Oh, good catch. I always wonder why I can perform a click faster than 300ms. Never thought it's 600ms (another 300ms from formUpdated: https://github.com/drupal/drupal/blob/8.3.x/core/misc/form.js#L182)
This issue only focuses on removing extra 300ms for non-transliteration. So I think it's good to go with your review. Right?
About the Drupal.behaviors.formUpdated, we can do in a follow-up issue. Not an easy job, though, I think we can make it optionally.
Comment #10
droplet commentedComment #11
michielnugter commentedCreating a separate issue for the debounce delay is the good way to go I think.
The issue is ok, I see no problems. As far as the OK for review, should this issue also be postponed untill the test is in?
I looked into adding this behavior into the testcase but because of the delay in debounce it's nearly impossible.
Comment #12
droplet commentedI think we don't need an extra JS test for this issue. The testbot doesn't always perform well and could be under 300ms. I will leave it to the committers to decide which one should get in first.
Comment #13
michielnugter commentedThen its RTBC I guess?
Comment #14
droplet commentedI don't know which one will go in first. Just jot a note to remove extra 300ms in test:
https://www.drupal.org/node/2821320#comment-11743898
I understand 300ms is nothing. but CLEAN CODE is CLEAN CODE. This is programming. :)
Comment #15
alexpottI think this behaviour can be tested and according to @droplet with this change we can change the tests added in #2821320: Add test coverage for machine-name.js
Comment #16
droplet commentedThanks @alexpott, The delay has removed in #2821320 last 2 patches already.
One reason I don't suggest to add test to catch this 300ms is the PhantomJS has random error (exceed 300ms for an action).
I re-run testbot once to verify this change will not break normal usages
Comment #17
alexpott@droplet so this patch doesn't add anything that is testable - that seems odd?
Comment #18
droplet commented@alexpott,
Yes, it's odd but I preferred no additional test to catch the delay on this issue. It will avoid random error FROM testbots. The basic feature has covered in other tests already.
Accurately, for this patch, we have to catch 1ms ~ 50ms response time (or loose value: 100ms~300ms), any higher than that meaningless.
Some of our developers' test env can't perform that well.
Here's one example I pointed out in test issue thread:
https://www.drupal.org/node/2782915#comment-11728915
@nickgs has 100% FAILED tests.
d.org has 10~30% FAILED
@droplet: 100% PASSED
Anyway, we can try if it's really needed.
Comment #19
droplet commentedI assigned it back to comitter to make the decision. The unstable random failure has more impact on d.org testbots than developers platform.
I'm fine to patch it and do wild experiment :)
Comment #21
alexpottCommitted 075aa45 and pushed to 8.3.x. Thanks!
If this does fix the random fails we should consider backporting to 8.2.x