Project

General

Profile

Anomalie #29372

BUG sur le script icon2SepTable.php : perte possible de l'ensemble des icones au reconfigure

Added by Renaud Dussol almost 3 years ago. Updated almost 3 years ago.

Status:
Fermé
Priority:
Haut
Assigned To:
Target version:
Start date:
12/09/2019
Due date:
% Done:

100%

Distribution:

Description

Tipunch sur IRC me fait remarquer que lorsqu'il fait un reconfigure, il perd les icones qu'il avait créées ou modifiées

Au départ je pensais que c'était un pb de config locale mais j'arrive à reproduire le pb chez moi en test (on ne fait pratiquement jamais de reconf)

Après analyse , le coupable serait le script utils/icon2SepTable.php

Ce script est censé modifier la structure de la table au cas ou les icones seraient toujours dans un champ "icon" de la table
Il transforme alors le champ "icon" en "old_icon", il crée une nouvelle entrée dans la table "icon" et il crée un nouveau champ "icon_id" qui fait référence à l'entrée créée précédemment dans cette table

Sauf que ce script commence par :

foreach ($apps as $app ) {
    if ($app->old_icon) {
        $icone = $app->old_icon;
    }

(Et pareil pour les urls plus bas)

Cela signifie que par défaut il va assigner à la nouvelle icone (celle qu'il va mettre dans la table "icon") la valeur de "old_icon"
Au début ce n'est pas grave : forcément, old_icon a d'abord la valeur de l'ancien icon
Mais si on change cette icone : ce sera icon_id qui changera. Mais PAS old_icon (on n'y touche plus). old_icon reste à l'icone initiale, c'est en quelque sorte un snapshot à un instant T de l'icone
Et si le script repasse derrière : il va remettre l'icone à old_icon qui est une ancienne version de l'icone...

Potentiellement c'est assez destructeur, car si un reconfigure est fait alors que beaucoup d’icônes ont été modifiées, celles-ci seront perdues

Je ne comprends pas pourquoi j'ai assigné cette valeur "old_icon" à $icone : cela n'a a priori aucun sens...
La seule explication que je vois, ce serait que j'avais prévu, lors d'une modif d'icone, dans le code applicatif, de modifier également old_icon.
Là, cela aurait un sens : on continue pendant quelques temps à faire cohabiter les 2 façons de faire : icone directement ds la table app + icone dans table séparée, mais je ne suis probablement pas allé jusqu'au bout

Ce qu'il faudrait faire, c'est simplement si on trouve le champ old_icon, on s'arrête, cela veut dire que le reste a été fait

foreach ($apps as $app ) {
    if ($app->old_icon) {
        break;
    }

Je t'assigne la demande pour avoir un avis sur la question, si des fois tu te souviens de quelque chose. Cela m'étonne qu'on ait laissé passer un truc aussi gros. Et cela m'étonne aussi qu'on n'ait eu aucune remontée depuis
D'un autre côté on ne fait pratiquement jamais de reconfigure en prod, j'imagine que les autres non plus...

Associated revisions

Revision c4239cef (diff)
Added by Renaud Dussol almost 3 years ago

Fixes #29372 : BUG sur le script icon2SepTable.php : perte possible de l'ensemble des icones au reconfigure

History

#1 Updated by Renaud Dussol almost 3 years ago

J'ai revu le script en totalité
Je fais d'abord un inspect des 2 tables, et si le champ icon_id est trouvé (signe qu'on est dans la nouvelle structure), on ne fait rien
Si le champ n'est pas trouvé, on parcours les apps et urls, on instancie le bean icon à partir du champ icon, ce qui crée automatiquement la relation icon_id-> table icon

Je me demande encore pourquoi j'avais fait un truc aussi compliqué et tordu... y a-t-il quelque chose qui m'échappe ?
Cela fait longtemps que je ne suis plus là-dessus donc je ne suis pas très sûr de moi...
En même temps comme ce bug vient de moi, à moi de le corriger !

Je commite la modif
Si tu as une minute, peux-tu le regarder pour voir si ça te semble ok ?
Je pense qu'il faudrait diffuser cela assez vite car c'est quand-même un gros gros bug...

#2 Updated by Renaud Dussol almost 3 years ago

  • Status changed from Nouveau to Résolu
  • % Done changed from 0 to 100

#3 Updated by Christophe LEON almost 3 years ago

Salut , Renaud

Désolé du retard, pour répondre à ta sollicitation,
j'ai relu le code, je n'ai pas relevé d'incohérence pour moi c'est OK et plus clair
il fait bien le boulot, en le lançant sur un env de test vu que la structure est déja créé il ne fait rien, donc c'est bon signe

Merci d'avoir regardé et corrigé,

Cdlt,
Christophe

#4 Updated by Renaud Dussol almost 3 years ago

OK super merci Christophe
J'ai remplacé mon script de prod par celui-ci, je te conseille de faire de même !
Et il faudra vite diffuser ou au moins prévenir les utilisateurs qui sont sous cette version et leur dire de mettre à jour manuellement le script ?
Je pense à Tipunch (dont je ne connais pas le vrai nom) et à Nicolas mais il y en a peut-être d'autres...

#5 Updated by Arnaud FORNEROT almost 3 years ago

  • Target version set to Envole 5.16

#6 Updated by Arnaud FORNEROT almost 3 years ago

  • Status changed from Résolu to Fermé

Also available in: Atom PDF