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.In the typical creation of an order, we do:
- Create and save an empty order, to get an order_id
- Create and save the line items
- Attach the line items to the order
- Save the order
During the save process, the order total gets computed, and we load from the database, even if we just saved them. I suggest that we should store the saved entity in the cache instead of simply clearing it.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 1363814-entity-controller-store-saved-static-cache_12.patch | 7.67 KB | amateescu |
| #12 | interdiff.txt | 6.08 KB | amateescu |
| #9 | 1363814-entity-controller-store-saved-static-cache.patch | 3.01 KB | Damien Tournoud |
| #6 | locking.patch | 2.64 KB | bojanz |
| #2 | 1363814-entity-controller-store-saved-static-cache.patch | 906 bytes | Damien Tournoud |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedTagging.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #4
bojanz CreditAttribution: bojanz commented#2: 1363814-entity-controller-store-saved-static-cache.patch queued for re-testing.
Comment #6
bojanz CreditAttribution: bojanz commentedSo, this goes against the pessimistic locking, since entityCache never gets emptied, the transaction never gets committed, and bad stuff happens.
There are two ways to solve this:
1) Only doing the performance optimization if pessimistic locking is disabled.
This is okay for our use case (where pessimistic locking might be disabled anyway), but it's still less than ideal.
2) The attached patch.
The problem with this patch is that it does exactly what we needed it to do.
Which means this:
a. You load an order. It gets locked.
b. You save an order. It gets unlocked but stays in entity cache.
c. You load an order again. It is now in an unlocked state, since it was fetched from entity cache.
This means that the pessimistic locking is no longer guaranteed.
Leaving the final call to Damien.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's see if this fixes the testing issue.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedRerolled.
Comment #11
bojanz CreditAttribution: bojanz commentedStill fails, it seems.
Comment #12
amateescu CreditAttribution: amateescu commented@bojanz forgot to empty the lockedEntities array if no arguments were passed to
resetCache(). This should fix the tests :)New patch and interdiff from #9.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedWoo. Great catch, @amateescu!
Comment #14
rszrama CreditAttribution: rszrama commentedCommitted with some minor whitespace changes. I don't see any direct answer to Bojan's question in #6 above with respect to orders "loaded" after a save being unlocked since they came from the cache. Based on Damien's RTBC, I assume this isn't a concern to him.
Thanks again to everyone for the group effort. Tests passed locally, too.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commented@rszrama: this patch doesn't change the behavior in this regard. We should deal with the remaining locking issues in a separate issue.