NTO-520 reworked url caching to avoid cache-misses and bugs #7
Conversation
|
|
||
| $elements = array_merge( | ||
| [SharedCacheHelper::ROUTING_NAMESPACE], | ||
| $tparams |
There was a problem hiding this comment.
This is the source of the problem - only the values in $tparams were being included in the cache key generation.
The result of this oversight meant that we'd get incorrect cache hits for multiple different instances of url_for(...). For example:
url_for('pack', ['id' => $order_id, 'confirmation' => 1, 'force' => 1])url_for('pack', ['id' => $order_id, 'undigital-flyer' => 1, 'force' => 1])
Both of these calls to url_for() would result in different final routes being generated if we were not caching. However, according to the previous caching logic, the cache keys for both these routes (and any in a similar format) would look something like: pack/[order_id]/1/1.
Whichever route had been cached under that key first would be what got returned for all similarly-formatted routes for the next three months, which is obviously nonsense.
| { | ||
| $elements = [ | ||
| SharedCacheHelper::ROUTING_NAMESPACE, | ||
| $this->flattenArrayElements($elements), |
There was a problem hiding this comment.
This fixes the problem. By flattening (with recursion, if necessary) both the keys and values down into a single numerically-indexed array, the above examples no longer cause incorrect cache hits with one another.
| $flattened[] = $key; | ||
|
|
||
| if (is_array($value)) { | ||
| $flattened[] = $this->flattenArrayElements($value); |
There was a problem hiding this comment.
This feels unlikely, but I wanted to add some degree of protection in just in case.
| protected function flattenArrayElements(array $elements = []): array | ||
| { | ||
| $flattened = []; | ||
| ksort($elements); |
There was a problem hiding this comment.
This will make sure we only cache a route once, even if the route arguments are given in a different order in different places. For example:
url_for('pack', ['id' => $order_id, 'confirmation' => 1, 'force' => 1])url_for('pack', ['id' => $order_id, 'force' => 1, 'confirmation' => 1])
These routes are fundamentally the same, and would result in the same output from the routing engine. To cache them separately because the arguments are provided in a different order would be foolish and wasteful.
| * Recursive method to pack a key => value pair array down into a flat | ||
| * numerically-indexed array, with a consistent ordering of elements. | ||
| */ | ||
| protected function flattenArrayElements(array $elements = []): array |
There was a problem hiding this comment.
Interesting. Should this method be in some sort of helper class?
There was a problem hiding this comment.
Umm....it's a good question, but I don't know if there are any other use cases for that functionality to be shared with.
I would definitely like to see the functionality shifted to a more centralised location if we have need for sharing it, but I would generally stick things close to where they're needed until we have a good reason to move it really.
No description provided.