Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.I've found that in acl_save_form() function the insert query will be executed multiple times in foreach loop. It is better to build the query in the loop and than execute it only once.
I've created a simple patch, which simply moves the query execution outside of the loop.
Please review.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | acl-save-form-insert-query-optimization-2650302-7.patch | 669 bytes | Elin Yordanov |
| #2 | acl-save-form-insert-query-optimization-2650302-2.patch | 659 bytes | Elin Yordanov |
Comments
Comment #2
Elin Yordanov CreditAttribution: Elin Yordanov commentedSorry for the first patch, it is not correct, please ignore it. I was too much in a hurry.
New patch is attached. Please review.
P.S.: Although the patch was not correct, it passed the tests, that means the tests are not complete.
Comment #3
salvisThank you for posting the issue and the patches, pc-wurm! No problem with posting them incrementally.
That's true. Actually, we have no tests at all for the embeddable form. That's why I hesitate to apply patches to code that has been working for years unless it can be shown that there's a problem with it...
I didn't know that db_insert() supported inserting multiple records at once, but your second patch certainly makes sense. Does it make a noticeable difference? Any reviewers/testers out there?
Would you like to contribute some tests for the embeddable form?
D7 is now legacy, and all fixes/patches need to go to D8 first (unless they don't apply), before they can be committed to D7.
Comment #4
Elin Yordanov CreditAttribution: Elin Yordanov commentedI'd like to contribute writing some tests, but it is not really my strength, sorry.
Comment #5
Elin Yordanov CreditAttribution: Elin Yordanov commentedHere's the patch re-rolled for 8.x-1.x-dev.
Please review.
Comment #7
Elin Yordanov CreditAttribution: Elin Yordanov commentedSorry, I forgot to split
fields()andvalues()for the query. Attached is a new patch.Please review.
Comment #9
salvis#2659944: Test failure: Adjust namespace for MigrateDrupal6TestBase base class for D81 needs to be fixed first.
Comment #11
salvisThe D8 failure is in the branch. I pushed the patch, thank you, Елин Й.!
(I'm sorry, I haven't been able to insert your name into my editor.)
Now to D7...
Comment #12
salvis#2 will work just fine.
Comment #13
gislePatch still need to be pushed to the repo. This must be done by a maintainer, not the author Elin Yordanov.
Comment #15
salvisThanks, gisle, I confused myself...
And thank you for the patch, Elin Yordanov!