From 6bb158fc2889b750aaeac6d9ff39ac4d7e9e400d Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Tue, 30 Apr 2019 14:22:37 +1200 Subject: [PATCH 1/4] More verbose file migration logging The log interval was useful to see errors in the noise, but it's also masked what the task actually does. Since this is moving and modifying project data, it should be verbose by default. This will help answer question later on, e.g. "why wasn't this file migrated". A new message at the bottom which spells out how many files failed (if any) makes it clear that you need to review the logs in detail (even though the error lines might hide between a lot of success lines). --- src/FileMigrationHelper.php | 40 ++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/src/FileMigrationHelper.php b/src/FileMigrationHelper.php index ed9691e1..ee8a7c99 100644 --- a/src/FileMigrationHelper.php +++ b/src/FileMigrationHelper.php @@ -40,14 +40,6 @@ class FileMigrationHelper extends BaseFileMigrationHelper */ private static $delete_invalid_files = true; - /** - * Specifies the interval for every Nth file looped at which output will be logged. - * - * @config - * @var int - */ - private static $log_interval = 10; - private static $dependencies = [ 'logger' => '%$' . LoggerInterface::class, ]; @@ -80,6 +72,7 @@ public function run($base = null) // If not, cannot migrate /** @skipUpgrade */ if (!DB::get_schema()->hasField('File', 'Filename')) { + $this->logger->error('Database not ready for migration. Please run dev/build'); return 0; } @@ -96,26 +89,39 @@ public function run($base = null) } $query = $this->getFileQuery(); - $total = $query->count(); + $totalCount = $query->count(); + + if (!$totalCount) { + return 0; + } + + $this->logger->info(sprintf('Migrating %d files', $totalCount)); + foreach ($query as $file) { // Bypass the accessor and the filename from the column $filename = $file->getField('Filename'); - $success = $this->migrateFile($base, $file, $filename); - if ($processedCount % $this->config()->get('log_interval') === 0) { - if ($this->logger) { - $this->logger->info("Iterated $processedCount out of $total files. Migrated $migratedCount files."); - } - } + $this->logger->info(sprintf('Migrating %s', $filename)); + + $success = $this->migrateFile($base, $file, $filename); $processedCount++; if ($success) { $migratedCount++; } } + if (class_exists(Versioned::class)) { Versioned::set_reading_mode($originalState); } + + if ($processedCount < $totalCount) { + $this->logger->warning(sprintf( + 'Failed to migrate %d files, please review errors', + $totalCount - $processedCount + )); + } + return $migratedCount; } @@ -179,7 +185,7 @@ protected function migrateFile($base, File $file, $legacyFilename) } $file = File::get_by_id($fileID); } - + if (!is_readable($path)) { if ($this->logger) { $this->logger->warning(sprintf( @@ -239,6 +245,8 @@ protected function migrateFile($base, File $file, $legacyFilename) } return $removed; } + + return true; } /** From a7b33b45cf2833674f7aafb4da63a7b1dafe4dd7 Mon Sep 17 00:00:00 2001 From: Aaron Carlino Date: Wed, 1 May 2019 16:09:57 +1200 Subject: [PATCH 2/4] Add quieter logging implementation, percentage outputs --- _config/migration.yml | 11 +++++++++++ src/FileMigrationHelper.php | 12 +++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-) create mode 100644 _config/migration.yml diff --git a/_config/migration.yml b/_config/migration.yml new file mode 100644 index 00000000..0e290c3e --- /dev/null +++ b/_config/migration.yml @@ -0,0 +1,11 @@ +--- +Name: file-migration +--- +# Remove the HTTPOutputHandler from the logger on the file migration. +# It logs a lot of >= NOTICE errors that pollute the output with backtrace. +SilverStripe\Core\Injector\Injector: + Psr\Log\LoggerInterface.quiet: + type: singleton + class: Monolog\Logger + constructor: + - "file-migration" \ No newline at end of file diff --git a/src/FileMigrationHelper.php b/src/FileMigrationHelper.php index ee8a7c99..77dfabe2 100644 --- a/src/FileMigrationHelper.php +++ b/src/FileMigrationHelper.php @@ -41,7 +41,7 @@ class FileMigrationHelper extends BaseFileMigrationHelper private static $delete_invalid_files = true; private static $dependencies = [ - 'logger' => '%$' . LoggerInterface::class, + 'logger' => '%$' . LoggerInterface::class . '.quiet', ]; /** @var LoggerInterface|null */ @@ -100,12 +100,18 @@ public function run($base = null) foreach ($query as $file) { // Bypass the accessor and the filename from the column $filename = $file->getField('Filename'); - $this->logger->info(sprintf('Migrating %s', $filename)); $success = $this->migrateFile($base, $file, $filename); $processedCount++; + $this->logger->info(sprintf( + '[%d / %d, %d%% complete]', + $processedCount, + $totalCount, + floor(($processedCount / $totalCount) * 100) + )); + if ($success) { $migratedCount++; } @@ -186,7 +192,7 @@ protected function migrateFile($base, File $file, $legacyFilename) $file = File::get_by_id($fileID); } - if (!is_readable($path)) { + if (!is_readable($path) || fopen($path, 'r') === false) { if ($this->logger) { $this->logger->warning(sprintf( 'File at %s is not readable and could not be migrated.', From c9b6383e3bfeb3a3e245bdb4e1cd5df56fa4e905 Mon Sep 17 00:00:00 2001 From: Aaron Carlino Date: Wed, 1 May 2019 16:36:46 +1200 Subject: [PATCH 3/4] Add fluent setter --- src/FileMigrationHelper.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/FileMigrationHelper.php b/src/FileMigrationHelper.php index 77dfabe2..f1caba10 100644 --- a/src/FileMigrationHelper.php +++ b/src/FileMigrationHelper.php @@ -49,10 +49,13 @@ class FileMigrationHelper extends BaseFileMigrationHelper /** * @param LoggerInterface $logger + * @return $this */ public function setLogger(LoggerInterface $logger) { $this->logger = $logger; + + return $this; } /** From de43d6d25e0292c7f4b321bb92e95ddc801cc255 Mon Sep 17 00:00:00 2001 From: Aaron Carlino Date: Wed, 1 May 2019 16:39:15 +1200 Subject: [PATCH 4/4] docblock formatting --- src/FileMigrationHelper.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/FileMigrationHelper.php b/src/FileMigrationHelper.php index f1caba10..f1758f05 100644 --- a/src/FileMigrationHelper.php +++ b/src/FileMigrationHelper.php @@ -44,7 +44,9 @@ class FileMigrationHelper extends BaseFileMigrationHelper 'logger' => '%$' . LoggerInterface::class . '.quiet', ]; - /** @var LoggerInterface|null */ + /** + * @var LoggerInterface|null + */ private $logger; /**