From a77832512d42e7712ade61b5fcabce0b16f281d4 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sat, 20 Nov 2021 21:08:32 +0300 Subject: [PATCH 01/11] pkp/pkp-lib#7167 Updated migration script and database creation --- .../migration/install/CommonMigration.inc.php | 4 +- .../upgrade/PKPv3_3_0UpgradeMigration.inc.php | 4 - ...tedUserSettingsAndDeprecatedFields.inc.php | 123 ++++++++++++++++++ .../v3_4_0/PreflightCheckMigration.inc.php | 5 +- 4 files changed, 128 insertions(+), 8 deletions(-) create mode 100644 classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php diff --git a/classes/migration/install/CommonMigration.inc.php b/classes/migration/install/CommonMigration.inc.php index b92871784e6..d3701b5982b 100644 --- a/classes/migration/install/CommonMigration.inc.php +++ b/classes/migration/install/CommonMigration.inc.php @@ -98,11 +98,9 @@ public function up(): void $table->bigInteger('user_id'); $table->string('locale', 14)->default(''); $table->string('setting_name', 255); - $table->bigInteger('assoc_type')->default(0); - $table->bigInteger('assoc_id')->default(0); $table->text('setting_value')->nullable(); $table->index(['user_id'], 'user_settings_user_id'); - $table->unique(['user_id', 'locale', 'setting_name', 'assoc_type', 'assoc_id'], 'user_settings_pkey'); + $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_pkey'); $table->index(['setting_name', 'locale'], 'user_settings_locale_setting_name_index'); }); diff --git a/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php b/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php index 3172e271ef2..2f221382768 100755 --- a/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php +++ b/classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php @@ -94,10 +94,6 @@ public function up(): void }); } - // Use nulls instead of 0 for assoc_type/id in user_settings - DB::table('user_settings')->where('assoc_type', 0)->update(['assoc_type' => null]); - DB::table('user_settings')->where('assoc_id', 0)->update(['assoc_id' => null]); - // pkp/pkp-lib#6093 Don't allow nulls (previously an upgrade workaround) Schema::table('announcement_types', function (Blueprint $table) { $table->bigInteger('assoc_type')->nullable(false)->change(); diff --git a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php new file mode 100644 index 00000000000..fe1262a7ddf --- /dev/null +++ b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php @@ -0,0 +1,123 @@ + 1 + ) AS duplicated + -- Find the best matching records for each entry + INNER JOIN user_settings best + ON best.setting_name = duplicated.setting_name + AND best.user_id = duplicated.user_id + AND best.locale = duplicated.locale + AND CONCAT(COALESCE(best.assoc_id, -9999), '@', COALESCE(best.assoc_type, -9999)) = ( + SELECT CONCAT(COALESCE(s.assoc_id, -9999), '@', COALESCE(s.assoc_type, -9999)) + FROM user_settings s + WHERE s.setting_name = duplicated.setting_name + AND s.user_id = duplicated.user_id + AND s.locale = duplicated.locale + ORDER BY + CASE s.assoc_id + WHEN 0 THEN 0 + WHEN NULL THEN 1 + ELSE 2 + END, + CASE s.assoc_type + WHEN 0 THEN 0 + WHEN NULL THEN 1 + ELSE 2 + END, s.assoc_id, s.assoc_type + LIMIT 1 + ) + ) best_duplicated + ) best_duplicated + -- The record matches the key fields, which means it's part of the duplicated set + ON s.setting_name = best_duplicated.setting_name + AND s.user_id = best_duplicated.user_id + AND s.locale = best_duplicated.locale + -- But unfortunately it's not the best match and thus will be removed + AND ( + COALESCE(s.assoc_id, -9999) <> COALESCE(best_duplicated.assoc_id, -9999) + OR COALESCE(s.assoc_type, -9999) <> COALESCE(best_duplicated.assoc_type, -9999) + )" + ); + + if ($isMysql) { + // Restore the optimizer setting to its default + DB::unprepared("SET optimizer_switch = 'derived_merge=default'"); + } + + // Here we should be free of duplicates, so it's safe to remove the columns without creating duplicated entries. + Schema::table( + 'user_settings', + function (Blueprint $table): void { + // Drop the unique index + $table->dropUnique('user_settings_pkey'); + // Drop deprecated fields + $table->dropColumn('assoc_id', 'assoc_type'); + // Add an ID field for the sake of normalization + $table->bigInteger('user_settings_id')->autoIncrement(); + // Restore the unique index, using the previous field order + $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_pkey'); + } + ); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + throw new Exception('Downgrade unsupported due to removed data'); + } +} diff --git a/classes/migration/upgrade/v3_4_0/PreflightCheckMigration.inc.php b/classes/migration/upgrade/v3_4_0/PreflightCheckMigration.inc.php index 8c7ba0f479c..7fdd6f1b73c 100755 --- a/classes/migration/upgrade/v3_4_0/PreflightCheckMigration.inc.php +++ b/classes/migration/upgrade/v3_4_0/PreflightCheckMigration.inc.php @@ -7,7 +7,7 @@ * Copyright (c) 2000-2021 John Willinsky * Distributed under the GNU GPL v3. For full terms see the file docs/COPYING. * - * @class I6093_AddForeignKeys + * @class PreflightCheckMigration * @brief Check for common problems early in the upgrade process. */ @@ -78,6 +78,9 @@ public function up(): void } } + /** + * Rollback the migrations. + */ public function down(): void { if ($fallbackVersion = $this->setFallbackVersion()) { From a33debb05599ae99ea627589568ef77c0ce1ba5a Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sat, 20 Nov 2021 21:57:32 +0300 Subject: [PATCH 02/11] pkp/pkp-lib#7167 Removed forgotten usage of the user_settings.setting_type --- classes/user/DAO.inc.php | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/classes/user/DAO.inc.php b/classes/user/DAO.inc.php index a583186db37..52b490cd8be 100644 --- a/classes/user/DAO.inc.php +++ b/classes/user/DAO.inc.php @@ -275,10 +275,10 @@ public function changeSitePrimaryLocale($oldLocale, $newLocale) // get all names of all users in the new locale $result = DB::select( 'SELECT DISTINCT us.user_id, usg.setting_value AS given_name, usf.setting_value AS family_name, usp.setting_value AS preferred_public_name - FROM user_settings us - LEFT JOIN user_settings usg ON (usg.user_id = us.user_id AND usg.locale = ? AND usg.setting_name = ?) - LEFT JOIN user_settings usf ON (usf.user_id = us.user_id AND usf.locale = ? AND usf.setting_name = ?) - LEFT JOIN user_settings usp ON (usp.user_id = us.user_id AND usp.locale = ? AND usp.setting_name = ?)', + FROM user_settings us + LEFT JOIN user_settings usg ON (usg.user_id = us.user_id AND usg.locale = ? AND usg.setting_name = ?) + LEFT JOIN user_settings usf ON (usf.user_id = us.user_id AND usf.locale = ? AND usf.setting_name = ?) + LEFT JOIN user_settings usp ON (usp.user_id = us.user_id AND usp.locale = ? AND usp.setting_name = ?)', [$newLocale, Identity::IDENTITY_SETTING_GIVENNAME, $newLocale, Identity::IDENTITY_SETTING_FAMILYNAME, $newLocale, 'preferredPublicName'] ); foreach ($result as $row) { @@ -287,20 +287,20 @@ public function changeSitePrimaryLocale($oldLocale, $newLocale) // if no user name exists in the new locale, insert them all foreach ($settingNames as $settingName) { DB::insert( - "INSERT INTO user_settings (user_id, locale, setting_name, setting_value, setting_type) - SELECT DISTINCT us.user_id, ?, ?, us.setting_value, 'string' - FROM user_settings us - WHERE us.setting_name = ? AND us.locale = ? AND us.user_id = ?", + "INSERT INTO user_settings (user_id, locale, setting_name, setting_value) + SELECT DISTINCT us.user_id, ?, ?, us.setting_value + FROM user_settings us + WHERE us.setting_name = ? AND us.locale = ? AND us.user_id = ?", [$newLocale, $settingName, $settingName, $oldLocale, $userId] ); } } elseif (empty($row->given_name)) { // if the given name does not exist in the new locale (but one of the other names do exist), insert it DB::insert( - "INSERT INTO user_settings (user_id, locale, setting_name, setting_value, setting_type) - SELECT DISTINCT us.user_id, ?, ?, us.setting_value, 'string' - FROM user_settings us - WHERE us.setting_name = ? AND us.locale = ? AND us.user_id = ?", + "INSERT INTO user_settings (user_id, locale, setting_name, setting_value) + SELECT DISTINCT us.user_id, ?, ?, us.setting_value + FROM user_settings us + WHERE us.setting_name = ? AND us.locale = ? AND us.user_id = ?", [$newLocale, Identity::IDENTITY_SETTING_GIVENNAME, Identity::IDENTITY_SETTING_GIVENNAME, $oldLocale, $userId] ); } From 1b3be7c7511b0cde9d68c7a05f738acb29ec18e7 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sat, 20 Nov 2021 23:57:35 +0300 Subject: [PATCH 03/11] pkp/pkp-lib#7167 Fixed query in PostgreSQL --- ...67_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php index fe1262a7ddf..1996e49100c 100644 --- a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php +++ b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php @@ -33,10 +33,12 @@ public function up(): void $isMysql = DB::connection() instanceof MySqlConnection; $deleteFrom = 'DELETE FROM user_settings s USING'; + $joinFilter = 'WHERE'; if ($isMysql) { // The optimizer might cut-off the sub-queries/closures below and make MySQL fail to delete from the table which is selecting from DB::unprepared("SET optimizer_switch = 'derived_merge=off'"); $deleteFrom = 'DELETE s FROM user_settings s INNER JOIN'; + $joinFilter = 'ON'; } // Locates and removes duplicated user_settings // The latest code stores settings using assoc_id = 0 and assoc_type = 0. Which means entries using null or anything else are outdated. @@ -82,7 +84,7 @@ public function up(): void ) best_duplicated ) best_duplicated -- The record matches the key fields, which means it's part of the duplicated set - ON s.setting_name = best_duplicated.setting_name + ${joinFilter} s.setting_name = best_duplicated.setting_name AND s.user_id = best_duplicated.user_id AND s.locale = best_duplicated.locale -- But unfortunately it's not the best match and thus will be removed From bd95bdd9a3970719630e4165415501a7bb5e4820 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sun, 21 Nov 2021 14:47:01 +0300 Subject: [PATCH 04/11] pkp/pkp-lib#7167 Updated the installation to not break when a downgrade fails --- .../DowngradeNotSupportedException.inc.php | 12 ++- classes/install/Installer.inc.php | 77 ++++++++++--------- .../v3_4_0/I6093_AddForeignKeys.inc.php | 3 +- ...tedUserSettingsAndDeprecatedFields.inc.php | 4 +- 4 files changed, 53 insertions(+), 43 deletions(-) diff --git a/classes/install/DowngradeNotSupportedException.inc.php b/classes/install/DowngradeNotSupportedException.inc.php index 5d20cb424cf..28618c5a487 100644 --- a/classes/install/DowngradeNotSupportedException.inc.php +++ b/classes/install/DowngradeNotSupportedException.inc.php @@ -12,10 +12,16 @@ namespace PKP\install; -class DowngradeNotSupportedException extends \Exception +use Exception; +use Throwable; + +class DowngradeNotSupportedException extends Exception { - public function __construct() + /** + * Constructor + */ + public function __construct(string $message = 'Downgrade not supported', int $code = 0, ?Throwable $previous = null) { - parent::__construct('Downgrade not supported!'); + parent::__construct($message, $code, $previous); } } diff --git a/classes/install/Installer.inc.php b/classes/install/Installer.inc.php index 243a788cfed..43a2e88f9a6 100644 --- a/classes/install/Installer.inc.php +++ b/classes/install/Installer.inc.php @@ -19,7 +19,7 @@ use APP\core\Application; use APP\file\LibraryFileManager; use APP\i18n\AppLocale; - +use Exception; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Schema; @@ -454,7 +454,7 @@ public function executeAction($action) try { $migration->up(); $this->migrations[] = $migration; - } catch (\Exception $e) { + } catch (Exception $e) { // Log an error message $this->setError( self::INSTALLER_ERROR_DB, @@ -466,7 +466,11 @@ public function executeAction($action) try { $this->log(sprintf('revert migration: %s', get_class($previousMigration))); $previousMigration->down(); - } catch (\PKP\install\DowngradeNotSupportedException $e) { + } catch (DowngradeNotSupportedException $e) { + $this->log(sprintf('downgrade for "%s" unsupported: %s', get_class($previousMigration), $e->getMessage())); + break; + } catch (Exception $e) { + $this->log(sprintf('error while downgrading "%s": %s', get_class($previousMigration), Config::getVar('debug', 'show_stacktrace') ? (string) $e : $e->getMessage())); break; } } @@ -725,8 +729,8 @@ public function setCurrentVersion($version) * * @param $installer object * @param $attr array Attributes: array containing - * 'key' => 'EMAIL_KEY_HERE', - * 'locales' => 'en_US,fr_CA,...' + * 'key' => 'EMAIL_KEY_HERE', + * f'locales' => 'en_US,fr_CA,...' */ public function installEmailTemplate($installer, $attr) { @@ -849,12 +853,12 @@ public function addPluginVersions() 0, 0, // Major, minor, revision, build Core::getCurrentDate(), // Date installed - 1, // Current + 1, // Current 'plugins.' . $category, // Type basename($plugin->getPluginPath()), // Product - '', // Class name - 0, // Lazy load - $plugin->isSitePlugin() // Site wide + '', // Class name + 0, // Lazy load + $plugin->isSitePlugin() // Site wide ); } $versionDao->insertVersion($pluginVersion, true); @@ -1028,15 +1032,16 @@ public function migrateMetadataSettings() foreach ($metadataSettings as $metadataSetting) { foreach ($contextIds as $contextId) { $result = $contextDao->retrieve( - ' - SELECT * FROM ' . $contextDao->settingsTableName . ' WHERE - ' . $contextDao->primaryKeyColumn . ' = ? - AND ( - setting_name = ? - OR setting_name = ? - OR setting_name = ? - ) - ', + 'SELECT * + FROM ' . $contextDao->settingsTableName . ' + WHERE + ' . $contextDao->primaryKeyColumn . ' = ? + AND ( + setting_name = ? + OR setting_name = ? + OR setting_name = ? + ) + ', [ $contextId, $metadataSetting . 'EnabledWorkflow', @@ -1057,13 +1062,12 @@ public function migrateMetadataSettings() if ($value !== METADATA_DISABLE) { $contextDao->update( - ' - INSERT INTO ' . $contextDao->settingsTableName . ' ( - ' . $contextDao->primaryKeyColumn . ', - locale, - setting_name, - setting_value - ) VALUES (?, ?, ?, ?)', + 'INSERT INTO ' . $contextDao->settingsTableName . ' ( + ' . $contextDao->primaryKeyColumn . ', + locale, + setting_name, + setting_value + ) VALUES (?, ?, ?, ?)', [ $contextId, '', @@ -1074,15 +1078,14 @@ public function migrateMetadataSettings() } $contextDao->update( - ' - DELETE FROM ' . $contextDao->settingsTableName . ' WHERE - ' . $contextDao->primaryKeyColumn . ' = ? - AND ( - setting_name = ? - OR setting_name = ? - OR setting_name = ? - ) - ', + 'DELETE FROM ' . $contextDao->settingsTableName . ' WHERE + ' . $contextDao->primaryKeyColumn . ' = ? + AND ( + setting_name = ? + OR setting_name = ? + OR setting_name = ? + ) + ', [ $contextId, $metadataSetting . 'EnabledWorkflow', @@ -1112,9 +1115,9 @@ public function setStatsEmailSettings() for ($users = $userGroupDao->getUsersById($userGroup->getId(), $context->getId()); $user = $users->next();) { $notificationSubscriptionSettingsDao->update( 'INSERT INTO notification_subscription_settings - (setting_name, setting_value, user_id, context, setting_type) - VALUES - (?, ?, ?, ?, ?)', + (setting_name, setting_value, user_id, context, setting_type) + VALUES + (?, ?, ?, ?, ?)', [ 'blocked_emailed_notification', PKPNotification::NOTIFICATION_TYPE_EDITORIAL_REPORT, diff --git a/classes/migration/upgrade/v3_4_0/I6093_AddForeignKeys.inc.php b/classes/migration/upgrade/v3_4_0/I6093_AddForeignKeys.inc.php index c5179b381f3..256e45f3a07 100755 --- a/classes/migration/upgrade/v3_4_0/I6093_AddForeignKeys.inc.php +++ b/classes/migration/upgrade/v3_4_0/I6093_AddForeignKeys.inc.php @@ -16,6 +16,7 @@ use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Schema; +use PKP\install\DowngradeNotSupportedException; class I6093_AddForeignKeys extends \PKP\migration\Migration { @@ -75,6 +76,6 @@ public function up(): void */ public function down(): void { - throw new \Exception('Downgrade unsupported due to removed data!'); + throw new DowngradeNotSupportedException('Downgrade unsupported due to removed data'); } } diff --git a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php index 1996e49100c..e0776880d50 100644 --- a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php +++ b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php @@ -13,11 +13,11 @@ namespace PKP\migration\upgrade\v3_4_0; -use Exception; use Illuminate\Database\MySqlConnection; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Schema; +use PKP\install\DowngradeNotSupportedException; use PKP\migration\Migration; class I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields extends Migration @@ -120,6 +120,6 @@ function (Blueprint $table): void { */ public function down(): void { - throw new Exception('Downgrade unsupported due to removed data'); + throw new DowngradeNotSupportedException('Downgrade unsupported due to removed data'); } } From f1068657a26aeddd4554d64a7e43f46a8460bcbd Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sun, 21 Nov 2021 18:04:27 +0300 Subject: [PATCH 05/11] pkp/pkp-lib#7167 Ignored failure when dropping the unique key (it will be dropped implicitly) --- classes/migration/install/CommonMigration.inc.php | 1 + ...licatedUserSettingsAndDeprecatedFields.inc.php | 15 +++++++++++---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/classes/migration/install/CommonMigration.inc.php b/classes/migration/install/CommonMigration.inc.php index d3701b5982b..f06e6eb864d 100644 --- a/classes/migration/install/CommonMigration.inc.php +++ b/classes/migration/install/CommonMigration.inc.php @@ -95,6 +95,7 @@ public function up(): void // Locale-specific user data Schema::create('user_settings', function (Blueprint $table) { + $table->bigInteger('user_settings_id')->autoIncrement(); $table->bigInteger('user_id'); $table->string('locale', 14)->default(''); $table->string('setting_name', 255); diff --git a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php index e0776880d50..d1f1bcc8450 100644 --- a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php +++ b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php @@ -13,6 +13,7 @@ namespace PKP\migration\upgrade\v3_4_0; +use Exception; use Illuminate\Database\MySqlConnection; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\DB; @@ -103,12 +104,18 @@ public function up(): void Schema::table( 'user_settings', function (Blueprint $table): void { - // Drop the unique index - $table->dropUnique('user_settings_pkey'); + try { + // Drop the unique index + $table->dropUnique('user_settings_pkey'); + } catch (Exception $e) { + error_log("Failed to gracefully drop the user_settings unique index, it will be dropped implicitly: ${e}"); + } // Drop deprecated fields $table->dropColumn('assoc_id', 'assoc_type'); - // Add an ID field for the sake of normalization - $table->bigInteger('user_settings_id')->autoIncrement(); + if (!Schema::hasColumn('user_settings', 'user_settings_id')) { + // Add an ID field for the sake of normalization + $table->bigInteger('user_settings_id')->autoIncrement(); + } // Restore the unique index, using the previous field order $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_pkey'); } From f6393af5d820fe015f626864723b4ef329be00ac Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sun, 21 Nov 2021 21:47:42 +0300 Subject: [PATCH 06/11] pkp/pkp-lib#7167 Prevented plugins from being re-registered, renamed unique key constraint to avoid conflicts --- classes/install/Installer.inc.php | 2 +- .../migration/install/CommonMigration.inc.php | 2 +- ...tedUserSettingsAndDeprecatedFields.inc.php | 10 ++------ classes/plugins/PluginRegistry.inc.php | 24 +++++++------------ 4 files changed, 13 insertions(+), 25 deletions(-) diff --git a/classes/install/Installer.inc.php b/classes/install/Installer.inc.php index 43a2e88f9a6..ce971dc30f1 100644 --- a/classes/install/Installer.inc.php +++ b/classes/install/Installer.inc.php @@ -730,7 +730,7 @@ public function setCurrentVersion($version) * @param $installer object * @param $attr array Attributes: array containing * 'key' => 'EMAIL_KEY_HERE', - * f'locales' => 'en_US,fr_CA,...' + * 'locales' => 'en_US,fr_CA,...' */ public function installEmailTemplate($installer, $attr) { diff --git a/classes/migration/install/CommonMigration.inc.php b/classes/migration/install/CommonMigration.inc.php index f06e6eb864d..888ee233c1a 100644 --- a/classes/migration/install/CommonMigration.inc.php +++ b/classes/migration/install/CommonMigration.inc.php @@ -101,7 +101,7 @@ public function up(): void $table->string('setting_name', 255); $table->text('setting_value')->nullable(); $table->index(['user_id'], 'user_settings_user_id'); - $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_pkey'); + $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_user_id_locale_setting_name'); $table->index(['setting_name', 'locale'], 'user_settings_locale_setting_name_index'); }); diff --git a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php index d1f1bcc8450..cfd3a2d9b9c 100644 --- a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php +++ b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php @@ -104,20 +104,14 @@ public function up(): void Schema::table( 'user_settings', function (Blueprint $table): void { - try { - // Drop the unique index - $table->dropUnique('user_settings_pkey'); - } catch (Exception $e) { - error_log("Failed to gracefully drop the user_settings unique index, it will be dropped implicitly: ${e}"); - } - // Drop deprecated fields + // Drop deprecated fields (this will implicitly drop the unique key "user_settings_pkey") $table->dropColumn('assoc_id', 'assoc_type'); if (!Schema::hasColumn('user_settings', 'user_settings_id')) { // Add an ID field for the sake of normalization $table->bigInteger('user_settings_id')->autoIncrement(); } // Restore the unique index, using the previous field order - $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_pkey'); + $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_user_id_locale_setting_name'); } ); } diff --git a/classes/plugins/PluginRegistry.inc.php b/classes/plugins/PluginRegistry.inc.php index bfb559e0ce7..fec2f284dd5 100644 --- a/classes/plugins/PluginRegistry.inc.php +++ b/classes/plugins/PluginRegistry.inc.php @@ -89,21 +89,12 @@ public static function register($category, $plugin, $path, $mainContextId = null $pluginName = $plugin->getName(); $plugins = & self::getPlugins(); - // Allow the plugin to register. - if (!$plugin->register($category, $path, $mainContextId)) { + // If the plugin is already loaded or failed/refused to register + if (isset($plugins[$category][$pluginName]) || !$plugin->register($category, $path, $mainContextId)) { return false; } - // If the plugin was already loaded, do not load it again. - if (isset($plugins[$category][$pluginName])) { - return false; - } - - if (isset($plugins[$category])) { - $plugins[$category][$pluginName] = $plugin; - } else { - $plugins[$category] = [$pluginName => $plugin]; - } + $plugins[$category][$pluginName] = $plugin; return true; } @@ -258,9 +249,12 @@ public static function getCategories() */ public static function loadAllPlugins($enabledOnly = false) { - // Retrieve and register categories (order is significant). - foreach (self::getCategories() as $category) { - self::loadCategory($category, $enabledOnly); + static $isLoaded; + if (!$isLoaded) { + // Retrieve and register categories (order is significant). + foreach (self::getCategories() as $category) { + self::loadCategory($category, $enabledOnly); + } } return self::getAllPlugins(); } From 8360e80b8afd2e4aba1976b45098016c557b9ce6 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Sun, 21 Nov 2021 23:02:22 +0300 Subject: [PATCH 07/11] pkp/pkp-lib#7167 Added fallback to dtd --- dtd/install.dtd | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dtd/install.dtd b/dtd/install.dtd index 6940607b235..32fdbef2044 100644 --- a/dtd/install.dtd +++ b/dtd/install.dtd @@ -44,7 +44,8 @@ condition CDATA #IMPLIED> + class CDATA #REQUIRED + fallback CDATA #IMPLIED> Date: Mon, 22 Nov 2021 10:44:25 +0300 Subject: [PATCH 08/11] pkp/pkp-lib#7167 Attempt to improve Travis performance --- tools/travis/install-composer-dependencies.sh | 2 +- tools/travis/php.ini | 8 ++++++++ tools/travis/prepare-webserver.sh | 1 + 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/tools/travis/install-composer-dependencies.sh b/tools/travis/install-composer-dependencies.sh index 92349a5c213..a41113220a5 100755 --- a/tools/travis/install-composer-dependencies.sh +++ b/tools/travis/install-composer-dependencies.sh @@ -12,4 +12,4 @@ set -e # Search for composer.json files, and run Composer to install the dependencies. -find . -maxdepth 4 -name composer.json -exec bash -c 'composer --no-ansi --working-dir="`dirname {}`" install' ";" +find . -maxdepth 4 -name composer.json -exec bash -c 'composer --no-ansi --working-dir="`dirname {}`" install --optimize-autoloader --classmap-authoritative' ";" diff --git a/tools/travis/php.ini b/tools/travis/php.ini index 46f5f3c7266..0335b5e1671 100644 --- a/tools/travis/php.ini +++ b/tools/travis/php.ini @@ -1 +1,9 @@ error_log = "error.log" +opcache.enable = 1 +opcache.enable_cli = 1 +opcache.validate_timestamps = 0 +opcache.memory_consumption = 128 +opcache.interned_strings_buffer = 16 +opcache.max_accelerated_files = 10000 +opcache.max_wasted_percentage = 10 +opcache.file_update_protection = 0 diff --git a/tools/travis/prepare-webserver.sh b/tools/travis/prepare-webserver.sh index c60aabda736..5c5fb67bb96 100755 --- a/tools/travis/prepare-webserver.sh +++ b/tools/travis/prepare-webserver.sh @@ -15,6 +15,7 @@ set -e if [ -z "$TRAVIS" ] ; then echo "(Skipping phpenv add)" else + phpenv config-rm xdebug.ini phpenv config-add lib/pkp/tools/travis/php.ini fi From de63bc6d0981a34650e9b247c6ad37209c56692f Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Tue, 23 Nov 2021 10:59:01 +0300 Subject: [PATCH 09/11] pkp/pkp-lib#7167 Reversed adding the user_settings_id field --- classes/migration/install/CommonMigration.inc.php | 3 +-- ...emoveDuplicatedUserSettingsAndDeprecatedFields.inc.php | 8 ++------ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/classes/migration/install/CommonMigration.inc.php b/classes/migration/install/CommonMigration.inc.php index 888ee233c1a..3aca767451a 100644 --- a/classes/migration/install/CommonMigration.inc.php +++ b/classes/migration/install/CommonMigration.inc.php @@ -95,13 +95,12 @@ public function up(): void // Locale-specific user data Schema::create('user_settings', function (Blueprint $table) { - $table->bigInteger('user_settings_id')->autoIncrement(); $table->bigInteger('user_id'); $table->string('locale', 14)->default(''); $table->string('setting_name', 255); $table->text('setting_value')->nullable(); $table->index(['user_id'], 'user_settings_user_id'); - $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_user_id_locale_setting_name'); + $table->primary(['user_id', 'locale', 'setting_name']); $table->index(['setting_name', 'locale'], 'user_settings_locale_setting_name_index'); }); diff --git a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php index cfd3a2d9b9c..7f695aefd2c 100644 --- a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php +++ b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php @@ -106,12 +106,8 @@ public function up(): void function (Blueprint $table): void { // Drop deprecated fields (this will implicitly drop the unique key "user_settings_pkey") $table->dropColumn('assoc_id', 'assoc_type'); - if (!Schema::hasColumn('user_settings', 'user_settings_id')) { - // Add an ID field for the sake of normalization - $table->bigInteger('user_settings_id')->autoIncrement(); - } - // Restore the unique index, using the previous field order - $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_user_id_locale_setting_name'); + // Restore the primary/unique index, using the previous field order + $table->primary(['user_id', 'locale', 'setting_name']); } ); } From aabc4c12c7372e02cc20531ed409a22ff02d42c8 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Wed, 24 Nov 2021 15:41:51 +0300 Subject: [PATCH 10/11] pkp/pkp-lib#7167 Updated SQL clause in order to avoid adding hacks for MySQL --- ...tedUserSettingsAndDeprecatedFields.inc.php | 118 ++++++++---------- 1 file changed, 55 insertions(+), 63 deletions(-) diff --git a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php index 7f695aefd2c..792ff7962e8 100644 --- a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php +++ b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php @@ -32,72 +32,64 @@ public function up(): void return; } - $isMysql = DB::connection() instanceof MySqlConnection; - $deleteFrom = 'DELETE FROM user_settings s USING'; - $joinFilter = 'WHERE'; - if ($isMysql) { - // The optimizer might cut-off the sub-queries/closures below and make MySQL fail to delete from the table which is selecting from - DB::unprepared("SET optimizer_switch = 'derived_merge=off'"); - $deleteFrom = 'DELETE s FROM user_settings s INNER JOIN'; - $joinFilter = 'ON'; - } // Locates and removes duplicated user_settings // The latest code stores settings using assoc_id = 0 and assoc_type = 0. Which means entries using null or anything else are outdated. - // Note: Old versions (e.g. OJS <= 2.x) made use of these fields, but these settings were removed years ago. - DB::unprepared( - "${deleteFrom} - ( - SELECT best_duplicated.* - FROM ( - SELECT best.* - -- Find the duplicated entries - FROM ( - SELECT s.setting_name, s.user_id, s.locale - FROM user_settings s - GROUP BY - s.setting_name, s.user_id, s.locale - HAVING COUNT(0) > 1 - ) AS duplicated - -- Find the best matching records for each entry - INNER JOIN user_settings best - ON best.setting_name = duplicated.setting_name - AND best.user_id = duplicated.user_id - AND best.locale = duplicated.locale - AND CONCAT(COALESCE(best.assoc_id, -9999), '@', COALESCE(best.assoc_type, -9999)) = ( - SELECT CONCAT(COALESCE(s.assoc_id, -9999), '@', COALESCE(s.assoc_type, -9999)) - FROM user_settings s - WHERE s.setting_name = duplicated.setting_name - AND s.user_id = duplicated.user_id - AND s.locale = duplicated.locale - ORDER BY - CASE s.assoc_id - WHEN 0 THEN 0 - WHEN NULL THEN 1 - ELSE 2 - END, - CASE s.assoc_type - WHEN 0 THEN 0 - WHEN NULL THEN 1 - ELSE 2 - END, s.assoc_id, s.assoc_type - LIMIT 1 + // Note: Old versions (e.g. OJS <= 2.x) made use of these fields to store some settings, but they have been removed years ago, which means they are safe to be discarded. + if (DB::connection() instanceof MySqlConnection) { + DB::unprepared( + "DELETE s + FROM user_settings s + -- Locates all duplicated settings (same key fields, except the assoc_type/assoc_id) + INNER JOIN user_settings duplicated + ON s.setting_name = duplicated.setting_name + AND s.user_id = duplicated.user_id + AND s.locale = duplicated.locale + AND ( + COALESCE(s.assoc_type, -999999) <> COALESCE(duplicated.assoc_type, -999999) + OR COALESCE(s.assoc_id, -999999) <> COALESCE(duplicated.assoc_id, -999999) + ) + -- Attempts to find a better fitting record among the duplicates (preference is given to the smaller assoc_id/assoc_type values) + LEFT JOIN user_settings best + ON best.setting_name = duplicated.setting_name + AND best.user_id = duplicated.user_id + AND best.locale = duplicated.locale + AND ( + COALESCE(best.assoc_id, 999999) < COALESCE(duplicated.assoc_id, 999999) + OR ( + COALESCE(best.assoc_id, 999999) = COALESCE(duplicated.assoc_id, 999999) + AND COALESCE(best.assoc_type, 999999) < COALESCE(duplicated.assoc_type, 999999) ) - ) best_duplicated - ) best_duplicated - -- The record matches the key fields, which means it's part of the duplicated set - ${joinFilter} s.setting_name = best_duplicated.setting_name - AND s.user_id = best_duplicated.user_id - AND s.locale = best_duplicated.locale - -- But unfortunately it's not the best match and thus will be removed - AND ( - COALESCE(s.assoc_id, -9999) <> COALESCE(best_duplicated.assoc_id, -9999) - OR COALESCE(s.assoc_type, -9999) <> COALESCE(best_duplicated.assoc_type, -9999) - )" - ); - - if ($isMysql) { - // Restore the optimizer setting to its default - DB::unprepared("SET optimizer_switch = 'derived_merge=default'"); + ) + -- Ensures a better record was found (if not found, it means the current duplicated record is the best and shouldn't be removed) + WHERE best.user_id IS NOT NULL" + ); + } else { + DB::unprepared( + "DELETE FROM user_settings s + USING user_settings duplicated + -- Attempts to find a better fitting record among the duplicates (preference is given to the smaller assoc_id/assoc_type values) + LEFT JOIN user_settings best + ON best.setting_name = duplicated.setting_name + AND best.user_id = duplicated.user_id + AND best.locale = duplicated.locale + AND ( + COALESCE(best.assoc_id, 999999) < COALESCE(duplicated.assoc_id, 999999) + OR ( + COALESCE(best.assoc_id, 999999) = COALESCE(duplicated.assoc_id, 999999) + AND COALESCE(best.assoc_type, 999999) < COALESCE(duplicated.assoc_type, 999999) + ) + ) + -- Locates all duplicated settings (same key fields, except the assoc_type/assoc_id) + WHERE s.setting_name = duplicated.setting_name + AND s.user_id = duplicated.user_id + AND s.locale = duplicated.locale + AND ( + COALESCE(s.assoc_type, -999999) <> COALESCE(duplicated.assoc_type, -999999) + OR COALESCE(s.assoc_id, -999999) <> COALESCE(duplicated.assoc_id, -999999) + ) + -- Ensures a better record was found (if not found, it means the current duplicated record is the best and shouldn't be removed) + AND best.user_id IS NOT NULL" + ); } // Here we should be free of duplicates, so it's safe to remove the columns without creating duplicated entries. From dd9b08a716e21f610775d171db97997ed41a9ea4 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Fri, 3 Dec 2021 17:08:31 +0300 Subject: [PATCH 11/11] pkp/pkp-lib#7167 Ensured index is removed in MySQL --- classes/migration/install/CommonMigration.inc.php | 2 +- ...7_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/classes/migration/install/CommonMigration.inc.php b/classes/migration/install/CommonMigration.inc.php index 3aca767451a..d3701b5982b 100644 --- a/classes/migration/install/CommonMigration.inc.php +++ b/classes/migration/install/CommonMigration.inc.php @@ -100,7 +100,7 @@ public function up(): void $table->string('setting_name', 255); $table->text('setting_value')->nullable(); $table->index(['user_id'], 'user_settings_user_id'); - $table->primary(['user_id', 'locale', 'setting_name']); + $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_pkey'); $table->index(['setting_name', 'locale'], 'user_settings_locale_setting_name_index'); }); diff --git a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php index 792ff7962e8..085590e4789 100644 --- a/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php +++ b/classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php @@ -13,7 +13,6 @@ namespace PKP\migration\upgrade\v3_4_0; -use Exception; use Illuminate\Database\MySqlConnection; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\DB; @@ -96,10 +95,10 @@ public function up(): void Schema::table( 'user_settings', function (Blueprint $table): void { - // Drop deprecated fields (this will implicitly drop the unique key "user_settings_pkey") + $table->dropIndex('user_settings_pkey'); $table->dropColumn('assoc_id', 'assoc_type'); // Restore the primary/unique index, using the previous field order - $table->primary(['user_id', 'locale', 'setting_name']); + $table->unique(['user_id', 'locale', 'setting_name'], 'user_settings_pkey'); } ); }