Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Internal: Renamed "settings_current" table to just "settings" - refs #5567 #5572

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

christianbeeznest
Copy link
Contributor

No description provided.

public/plugin/bbb/lib/bbb_plugin.class.php Show resolved Hide resolved
public/main/admin/configure_extensions.php Show resolved Hide resolved
public/main/admin/settings.php Show resolved Hide resolved
public/main/admin/languages.php Show resolved Hide resolved
public/main/admin/settings.lib.php Show resolved Hide resolved
Copy link

codeclimate bot commented Jun 5, 2024

Code Climate has analyzed commit bf239c3 and detected 29 issues on this pull request.

Here's the issue category breakdown:

Category Count
Style 25
Clarity 2
Bug Risk 2

View more on Code Climate.

@ywarnier
Copy link
Member

ywarnier commented Jul 3, 2024

Bueno... técnicamente no era necesario renombrar la constante TABLE_MAIN_SETTINGS_CURRENT (justamente para eso usamos una constante, para no tener luego que cambiar el texto por todos lados), solo su valor, pero ahora que está hecho, déjalo así.
A cambio dentro de plugin.class.php::addTab(), no se debería usar directamente el nombre de la tabla (settings), sino que debería pasar por Database::get_main_table()

@ywarnier
Copy link
Member

ywarnier commented Jul 3, 2024

When I try to execute the migration, I get a "create" and a "drop" table, not a rename, so all settings would be lost:

# php bin/console doctrine:schema:update --dump-sql --complete
CREATE TABLE settings (id INT AUTO_INCREMENT NOT NULL, access_url INT DEFAULT NULL, variable VARCHAR(190) NOT NULL, subkey VARCHAR(190) DEFAULT NULL, type VARCHAR(255) DEFAULT NULL, category VARCHAR(255) DEFAULT NULL, selected_value LONGTEXT DEFAULT NULL, title LONGTEXT NOT NULL, comment LONGTEXT DEFAULT NULL, scope VARCHAR(50) DEFAULT NULL, subkeytext VARCHAR(255) DEFAULT NULL, access_url_changeable INT NOT NULL, access_url_locked INT DEFAULT 0 NOT NULL, INDEX access_url (access_url), UNIQUE INDEX unique_setting (variable, subkey, access_url), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci` ENGINE = InnoDB ROW_FORMAT = DYNAMIC;
ALTER TABLE settings ADD CONSTRAINT FK_E545A0C59436187B FOREIGN KEY (access_url) REFERENCES access_url (id);
ALTER TABLE settings_current DROP FOREIGN KEY FK_62F79C3B9436187B;
DROP TABLE settings_current;

Maybe testing it we realise that the --dump-sql command is badly documented and it replaces the table, but for what I see this is not the case.

@ywarnier
Copy link
Member

ywarnier commented Jul 3, 2024

Otherwise this PR is OK.

@ywarnier
Copy link
Member

ywarnier commented Jul 4, 2024

The migration can be applied (on existing development systems) with:

@ywarnier ywarnier merged commit fe97f8d into chamilo:master Jul 4, 2024
3 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants