Skip to content

Commit

Permalink
Merge pull request pkp#7464 from jonasraoni/bugfix/master/7167-fix-us…
Browse files Browse the repository at this point in the history
…er-settings

pkp#7167 Fix duplicated user settings and remove unused fields
  • Loading branch information
asmecher authored Dec 8, 2021
2 parents 39bc7bf + dd9b08a commit 46d1226
Show file tree
Hide file tree
Showing 13 changed files with 202 additions and 78 deletions.
12 changes: 9 additions & 3 deletions classes/install/DowngradeNotSupportedException.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
77 changes: 40 additions & 37 deletions classes/install/Installer.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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',
* 'locales' => 'en_US,fr_CA,...'
*/
public function installEmailTemplate($installer, $attr)
{
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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',
Expand All @@ -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,
'',
Expand All @@ -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',
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 1 addition & 3 deletions classes/migration/install/CommonMigration.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});

Expand Down
4 changes: 0 additions & 4 deletions classes/migration/upgrade/PKPv3_3_0UpgradeMigration.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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');
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
<?php

/**
* @file classes/migration/upgrade/v3_4_0/I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields.inc.php
*
* Copyright (c) 2014-2021 Simon Fraser University
* Copyright (c) 2000-2021 John Willinsky
* Distributed under the GNU GPL v3. For full terms see the file docs/COPYING.
*
* @class I7167_RemoveDuplicatedUserSettingsAndDeprecatedFields
* @brief Describe upgrade/downgrade Removes duplicated user_settings, as well as the deprecated fields "assoc_id" and "assoc_type".
*/

namespace PKP\migration\upgrade\v3_4_0;

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
{
/**
* Run the migrations.
*/
public function up(): void
{
if (!Schema::hasColumn('user_settings', 'assoc_id') && !Schema::hasColumn('user_settings', 'assoc_type')) {
return;
}

// 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 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)
)
)
-- 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.
Schema::table(
'user_settings',
function (Blueprint $table): void {
$table->dropIndex('user_settings_pkey');
$table->dropColumn('assoc_id', 'assoc_type');
// Restore the primary/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 DowngradeNotSupportedException('Downgrade unsupported due to removed data');
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/

Expand Down Expand Up @@ -78,6 +78,9 @@ public function up(): void
}
}

/**
* Rollback the migrations.
*/
public function down(): void
{
if ($fallbackVersion = $this->setFallbackVersion()) {
Expand Down
24 changes: 9 additions & 15 deletions classes/plugins/PluginRegistry.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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();
}
Expand Down
Loading

0 comments on commit 46d1226

Please sign in to comment.