chamilo / chamilo-lms

Chamilo is a learning management system focused on ease of use and accessibility
https://chamilo.org
GNU General Public License v3.0
792 stars 478 forks source link

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

Closed christianbeeznest closed 3 months ago

codeclimate[bot] commented 3 months ago

Code Climate has analyzed commit bf239c35 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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

Otherwise this PR is OK.

ywarnier commented 3 months ago

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