From 56d63f3f055bba5be6b6b02fb3ed72318b8f0b95 Mon Sep 17 00:00:00 2001 From: Jaap Jansma Date: Sat, 28 Mar 2020 21:16:04 +0100 Subject: [PATCH 1/4] dev-1674: fix rebuilding of container on every request --- CRM/Api4/Services.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CRM/Api4/Services.php b/CRM/Api4/Services.php index ec8190348ccd..736866af6a39 100644 --- a/CRM/Api4/Services.php +++ b/CRM/Api4/Services.php @@ -83,7 +83,7 @@ public static function loadServices($namespace, $tag, $container) { $path = \CRM_Utils_File::addTrailingSlash(dirname($location)) . str_replace('\\', DIRECTORY_SEPARATOR, $namespace); try { $resource = new \Symfony\Component\Config\Resource\DirectoryResource($path, ';\.php$;'); - $container->addResource($resource); + $addResource = false; foreach (glob("$path*.php") as $file) { $matches = []; preg_match('/(\w*)\.php$/', $file, $matches); @@ -92,8 +92,12 @@ public static function loadServices($namespace, $tag, $container) { if ($serviceClass->isInstantiable()) { $definition = $container->register(str_replace('\\', '_', $serviceName), $serviceName); $definition->addTag($tag); + $addResource = true; } } + if ($addResource) { + $container->addResource($resource); + } } catch (\InvalidArgumentException $e) { //Directory is not found so lets not do anything i suppose. From 61d72c88752a3fa91447d2a44ff563b2c4c1c11d Mon Sep 17 00:00:00 2001 From: Jaap Jansma Date: Sat, 28 Mar 2020 22:55:57 +0100 Subject: [PATCH 2/4] Update style warnings --- CRM/Api4/Services.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CRM/Api4/Services.php b/CRM/Api4/Services.php index 736866af6a39..c00487df5f89 100644 --- a/CRM/Api4/Services.php +++ b/CRM/Api4/Services.php @@ -83,7 +83,7 @@ public static function loadServices($namespace, $tag, $container) { $path = \CRM_Utils_File::addTrailingSlash(dirname($location)) . str_replace('\\', DIRECTORY_SEPARATOR, $namespace); try { $resource = new \Symfony\Component\Config\Resource\DirectoryResource($path, ';\.php$;'); - $addResource = false; + $addResource = FALSE; foreach (glob("$path*.php") as $file) { $matches = []; preg_match('/(\w*)\.php$/', $file, $matches); @@ -92,7 +92,7 @@ public static function loadServices($namespace, $tag, $container) { if ($serviceClass->isInstantiable()) { $definition = $container->register(str_replace('\\', '_', $serviceName), $serviceName); $definition->addTag($tag); - $addResource = true; + $addResource = TRUE; } } if ($addResource) { From 5f16ea7b5b1d4edcd4be21bf7963dafead9686ea Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Wed, 1 Apr 2020 01:00:07 -0700 Subject: [PATCH 3/4] dev/core#1674 - Update container cache when folder is removed or created Conceptually, you want to: * (A) Keep the container cache when nothing has changed (regardless of whether that status-quo has an API4 subscriber folder). * (B) Update the cache if Civi/Api4/Event/Subscriber/*.php has been newly created * (C) Update the cache if Civi/Api4/Event/Subscriber/*.php has been newly removed To test this out, I hacked `Civi\Core\Container::loadContainer()` to emit a debug message to indicate if it is reusing the cache or updating. Then, I prepared two terminals for running alternate steps: * In terminal 1, go back and forth with adding/removing folders/files like `Civi/Api4/Event/Subscriber`. * In terminal 2, periodically run `cv ev 'echo "Hello\n";'` and note whether the cache is update. In this way, we can see if a developer action (adding a file) leads to an automatic update in the cache. I found that the previous commit fixed (A) and did (C), but it failed to update per (B). This commit should handle (A), (B), and (C). --- CRM/Api4/Services.php | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/CRM/Api4/Services.php b/CRM/Api4/Services.php index c00487df5f89..fba89627dd9b 100644 --- a/CRM/Api4/Services.php +++ b/CRM/Api4/Services.php @@ -81,9 +81,14 @@ public static function loadServices($namespace, $tag, $container) { ); foreach ($locations as $location) { $path = \CRM_Utils_File::addTrailingSlash(dirname($location)) . str_replace('\\', DIRECTORY_SEPARATOR, $namespace); + if (!file_exists($path) || !is_dir($path)) { + $resource = new \Symfony\Component\Config\Resource\FileExistenceResource($path); + $container->addResource($resource); + continue; + } + try { $resource = new \Symfony\Component\Config\Resource\DirectoryResource($path, ';\.php$;'); - $addResource = FALSE; foreach (glob("$path*.php") as $file) { $matches = []; preg_match('/(\w*)\.php$/', $file, $matches); @@ -92,15 +97,18 @@ public static function loadServices($namespace, $tag, $container) { if ($serviceClass->isInstantiable()) { $definition = $container->register(str_replace('\\', '_', $serviceName), $serviceName); $definition->addTag($tag); - $addResource = TRUE; } } - if ($addResource) { - $container->addResource($resource); - } + $container->addResource($resource); } catch (\InvalidArgumentException $e) { //Directory is not found so lets not do anything i suppose. + + // FIXME: The above comment implies that the try/catch is specifically + // about the directory's existence, which would make it redundant with the + // newer "file_exists()" guard. However, the actual "catch()" seems broader, + // and I don't see anything in DirectoryResource that throws the exception. + // So... maybe it's not needed, or maybe there's some additional purpose. } } } From 3733aeb0ce49cbfe1af351a8f0cefb5bcdf1ad6c Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Wed, 1 Apr 2020 15:03:33 -0700 Subject: [PATCH 4/4] (REF) CRM_Api4_Services - Cleanup extra comment/try/catch --- CRM/Api4/Services.php | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/CRM/Api4/Services.php b/CRM/Api4/Services.php index fba89627dd9b..35c24449e04a 100644 --- a/CRM/Api4/Services.php +++ b/CRM/Api4/Services.php @@ -84,10 +84,8 @@ public static function loadServices($namespace, $tag, $container) { if (!file_exists($path) || !is_dir($path)) { $resource = new \Symfony\Component\Config\Resource\FileExistenceResource($path); $container->addResource($resource); - continue; } - - try { + else { $resource = new \Symfony\Component\Config\Resource\DirectoryResource($path, ';\.php$;'); foreach (glob("$path*.php") as $file) { $matches = []; @@ -101,15 +99,6 @@ public static function loadServices($namespace, $tag, $container) { } $container->addResource($resource); } - catch (\InvalidArgumentException $e) { - //Directory is not found so lets not do anything i suppose. - - // FIXME: The above comment implies that the try/catch is specifically - // about the directory's existence, which would make it redundant with the - // newer "file_exists()" guard. However, the actual "catch()" seems broader, - // and I don't see anything in DirectoryResource that throws the exception. - // So... maybe it's not needed, or maybe there's some additional purpose. - } } }