From 7d64ff2af391fc767a9551d158771b53e3d13588 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 29 Jul 2026 22:30:17 +0600 Subject: [PATCH 1/3] perf: memoize GridConfigService::getGridConfig per request Cache grid config by gridKey and includeHidden on the service instance, and invalidate entries when grid config is mutated in the same request. Closes #352 --- .../src/Services/GridConfigService.php | 40 ++++++++++++++ .../Services/GridConfigServiceMemoTest.php | 53 +++++++++++++++++++ .../tests/stubs/GridConfigModxStub.php | 53 +++++++++++++++++++ .../tests/stubs/GridConfigQueryStub.php | 26 +++++++++ 4 files changed, 172 insertions(+) create mode 100644 core/components/minishop3/tests/Unit/Services/GridConfigServiceMemoTest.php create mode 100644 core/components/minishop3/tests/stubs/GridConfigModxStub.php create mode 100644 core/components/minishop3/tests/stubs/GridConfigQueryStub.php diff --git a/core/components/minishop3/src/Services/GridConfigService.php b/core/components/minishop3/src/Services/GridConfigService.php index e1d913d8..bae461b1 100644 --- a/core/components/minishop3/src/Services/GridConfigService.php +++ b/core/components/minishop3/src/Services/GridConfigService.php @@ -14,6 +14,9 @@ class GridConfigService /** @var modX */ protected $modx; + /** @var array>> Request-scoped memo: gridKey:includeHidden → config */ + private array $gridConfigCache = []; + /** * @param modX $modx */ @@ -29,6 +32,22 @@ public function __construct(modX $modx) * @return array */ public function getGridConfig(string $gridKey, bool $includeHidden = false): array + { + $cacheKey = $this->gridConfigCacheKey($gridKey, $includeHidden); + if (array_key_exists($cacheKey, $this->gridConfigCache)) { + return $this->gridConfigCache[$cacheKey]; + } + + $fields = $this->loadGridConfig($gridKey, $includeHidden); + $this->gridConfigCache[$cacheKey] = $fields; + + return $fields; + } + + /** + * @return list> + */ + private function loadGridConfig(string $gridKey, bool $includeHidden): array { $query = $this->modx->newQuery(msGridField::class); $where = ['grid_key' => $gridKey]; @@ -74,6 +93,19 @@ public function getGridConfig(string $gridKey, bool $includeHidden = false): arr return $fields; } + private function gridConfigCacheKey(string $gridKey, bool $includeHidden): string + { + return $gridKey . ':' . ($includeHidden ? '1' : '0'); + } + + private function invalidateGridConfigCache(string $gridKey): void + { + unset( + $this->gridConfigCache[$this->gridConfigCacheKey($gridKey, false)], + $this->gridConfigCache[$this->gridConfigCacheKey($gridKey, true)] + ); + } + /** * Get label considering lexicon_key * @@ -112,6 +144,8 @@ protected function resolveLabel(msGridField $field): string */ public function saveGridConfig(string $gridKey, array $fields): bool { + $this->invalidateGridConfigCache($gridKey); + try { // Get list of field names to keep $fieldNamesToKeep = []; @@ -253,6 +287,8 @@ public function saveGridConfig(string $gridKey, array $fields): bool */ public function deleteField(string $gridKey, string $fieldName): array { + $this->invalidateGridConfigCache($gridKey); + try { $field = $this->modx->getObject(msGridField::class, [ 'grid_key' => $gridKey, @@ -308,6 +344,8 @@ public function deleteField(string $gridKey, string $fieldName): array */ public function addField(string $gridKey, array $data): array { + $this->invalidateGridConfigCache($gridKey); + try { // Basic validation if (empty($data['field_name'])) { @@ -450,6 +488,8 @@ public function addField(string $gridKey, array $data): array */ public function updateField(string $gridKey, string $fieldName, array $data): array { + $this->invalidateGridConfigCache($gridKey); + try { // Find existing field $field = $this->modx->getObject(msGridField::class, [ diff --git a/core/components/minishop3/tests/Unit/Services/GridConfigServiceMemoTest.php b/core/components/minishop3/tests/Unit/Services/GridConfigServiceMemoTest.php new file mode 100644 index 00000000..73345b16 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/GridConfigServiceMemoTest.php @@ -0,0 +1,53 @@ +getGridConfig('orders', true); + $second = $service->getGridConfig('orders', true); + + self::assertSame($first, $second); + self::assertSame(1, $modx->getCollectionCalls); + self::assertSame([], $first); + } + + public function testIncludeHiddenUsesSeparateCacheEntry(): void + { + $modx = new GridConfigModxStub(); + $service = new GridConfigService($modx); + + $service->getGridConfig('orders', false); + $service->getGridConfig('orders', true); + + self::assertSame(2, $modx->getCollectionCalls); + } + + public function testSaveGridConfigInvalidatesMemoForSameRequest(): void + { + $modx = new GridConfigModxStub(); + $service = new GridConfigService($modx); + + $service->getGridConfig('orders', true); + $service->saveGridConfig('orders', []); + $service->getGridConfig('orders', true); + + // 1st read + saveGridConfig delete scan + 2nd read after cache invalidation + self::assertSame(3, $modx->getCollectionCalls); + } +} diff --git a/core/components/minishop3/tests/stubs/GridConfigModxStub.php b/core/components/minishop3/tests/stubs/GridConfigModxStub.php new file mode 100644 index 00000000..498cca90 --- /dev/null +++ b/core/components/minishop3/tests/stubs/GridConfigModxStub.php @@ -0,0 +1,53 @@ + */ + public array $gridFields = []; + + /** @var object */ + public object $lexicon; + + public function __construct() + { + parent::__construct(); + $this->lexicon = new class { + public function load(string $topic): void + { + } + + public function __invoke(string $key): string + { + return $key; + } + }; + } + + public function newQuery($class = ''): GridConfigQueryStub + { + return new GridConfigQueryStub(); + } + + public function getCollection($className, $criteria = null): array + { + $this->getCollectionCalls++; + + return $this->gridFields; + } + + public function getObject($className, $criteria = null, $cacheFlag = true): ?object + { + return null; + } +} diff --git a/core/components/minishop3/tests/stubs/GridConfigQueryStub.php b/core/components/minishop3/tests/stubs/GridConfigQueryStub.php new file mode 100644 index 00000000..0d16ea35 --- /dev/null +++ b/core/components/minishop3/tests/stubs/GridConfigQueryStub.php @@ -0,0 +1,26 @@ + Date: Wed, 29 Jul 2026 23:01:54 +0600 Subject: [PATCH 2/3] docs(grid-config): document request-scoped singleton invariant Expand the gridConfigCache docblock to state why the memo is safe only under the DI singleton (one instance per request, no cross-request persistence) and that all four mutation methods invalidate via invalidateGridConfigCache(). --- .../minishop3/src/Services/GridConfigService.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/core/components/minishop3/src/Services/GridConfigService.php b/core/components/minishop3/src/Services/GridConfigService.php index bae461b1..7d0e8d37 100644 --- a/core/components/minishop3/src/Services/GridConfigService.php +++ b/core/components/minishop3/src/Services/GridConfigService.php @@ -14,7 +14,20 @@ class GridConfigService /** @var modX */ protected $modx; - /** @var array>> Request-scoped memo: gridKey:includeHidden → config */ + /** + * Request-scoped memo for getGridConfig(): gridKey:includeHidden → column config. + * + * This cache is safe only because GridConfigService is registered in the + * MODX DI container as a request-scoped singleton (see ServiceRegistry): + * one instance per request, never shared across requests. The memo must + * never be persisted to a long-lived process cache or serialized — it + * holds no TTL and would leak stale data across requests. Mutation + * methods (saveGridConfig, addField, updateField, deleteField) invalidate + * the affected gridKey entries via invalidateGridConfigCache() so the next + * read re-loads from the database. + * + * @var array>> + */ private array $gridConfigCache = []; /** From e0bbb738cb7becdd1cabe63ddaf8da92f12e168a Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 29 Jul 2026 23:30:44 +0600 Subject: [PATCH 3/3] refactor(grid-config): route mutations through beginGridMutation helper Single entry point for cache invalidation before msGridField writes. --- .../minishop3/src/Services/GridConfigService.php | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/core/components/minishop3/src/Services/GridConfigService.php b/core/components/minishop3/src/Services/GridConfigService.php index 7d0e8d37..014d00ea 100644 --- a/core/components/minishop3/src/Services/GridConfigService.php +++ b/core/components/minishop3/src/Services/GridConfigService.php @@ -111,6 +111,14 @@ private function gridConfigCacheKey(string $gridKey, bool $includeHidden): strin return $gridKey . ':' . ($includeHidden ? '1' : '0'); } + /** + * Call at the start of any msGridField mutation so request-scoped cache stays consistent. + */ + private function beginGridMutation(string $gridKey): void + { + $this->invalidateGridConfigCache($gridKey); + } + private function invalidateGridConfigCache(string $gridKey): void { unset( @@ -157,7 +165,7 @@ protected function resolveLabel(msGridField $field): string */ public function saveGridConfig(string $gridKey, array $fields): bool { - $this->invalidateGridConfigCache($gridKey); + $this->beginGridMutation($gridKey); try { // Get list of field names to keep @@ -300,7 +308,7 @@ public function saveGridConfig(string $gridKey, array $fields): bool */ public function deleteField(string $gridKey, string $fieldName): array { - $this->invalidateGridConfigCache($gridKey); + $this->beginGridMutation($gridKey); try { $field = $this->modx->getObject(msGridField::class, [ @@ -357,7 +365,7 @@ public function deleteField(string $gridKey, string $fieldName): array */ public function addField(string $gridKey, array $data): array { - $this->invalidateGridConfigCache($gridKey); + $this->beginGridMutation($gridKey); try { // Basic validation @@ -501,7 +509,7 @@ public function addField(string $gridKey, array $data): array */ public function updateField(string $gridKey, string $fieldName, array $data): array { - $this->invalidateGridConfigCache($gridKey); + $this->beginGridMutation($gridKey); try { // Find existing field