Modification du fichier d'un document #4898

Closed
maieul wants to merge 1 commits from maieul/medias:issue4872_perte_lien into master
maieul commented 3 months ago
Collaborator

Cette PR :

  • résoud #4872 : lorsqu'on modifie un fichier d'un document, ne pas perdre les liens du document
  • supprime du code mort (formulaire MODIFIER_FICHIER_DOCUMENT)
Cette PR : - résoud #4872 : lorsqu'on modifie un fichier d'un document, ne pas perdre les liens du document - <del>supprime du code mort (formulaire `MODIFIER_FICHIER_DOCUMENT`)</del>
maieul added 2 commits 3 months ago
rastapopoulos reviewed 3 months ago
rastapopoulos left a comment
Owner

Peut-être que le formulaire déprécié était toujours là… volontairement car même déprécié, cela permet de ne pas casser ceux qui l'utilisaient à part. Je ne sais pas hein, mais faudrait déjà demander si c'était voulu.

Quoiqu'il en soit, je ne pense pas que supprimer/nettoyer des trucs dépréciés doit être mélangé dans la même PR qu'une correction de bug s'il n'y aucun lien direct (= si ce n'est pas justifié pour corriger le bug en question). Sinon on va moins facilement retrouver où et pourquoi ça a été enlevé, alors que ça n'avait pas de rapport.

Peut-être que le formulaire déprécié était toujours là… volontairement car même déprécié, cela permet de ne pas casser ceux qui l'utilisaient à part. Je ne sais pas hein, mais faudrait déjà demander si c'était voulu. Quoiqu'il en soit, je ne pense pas que supprimer/nettoyer des trucs dépréciés doit être mélangé dans la même PR qu'une correction de bug s'il n'y aucun lien direct (= si ce n'est pas justifié pour corriger le bug en question). Sinon on va moins facilement retrouver où et pourquoi ça a été enlevé, alors que ça n'avait pas de rapport.
or _request('joindre_zip')
) {
$modifier_fichier = true;
set_request('parents');
Owner

à quoi sert ce set_request puisque tu refais le même hors du if (donc valant aussi pour si on était dans ce if) juste 2 lignes après ?

à quoi sert ce set_request puisque tu refais le même *hors* du if (donc valant aussi pour si on était dans ce if) juste 2 lignes après ?
Poster
Collaborator

Hors du if, mais après formulaires_editer_objet_traiter, donc après qu'on ait modifié l'objet document, si besoin (par ex en modifiant depuis le formulaire d'édition du document les parents).

Hors du if, mais après `formulaires_editer_objet_traiter`, donc après qu'on ait modifié l'objet document, si besoin (par ex en modifiant depuis le formulaire d'édition du document les parents).
Owner

Ah oui ok compris dsl

Ah oui ok compris dsl
rastapopoulos marked this conversation as resolved
Poster
Collaborator

Peut-être que le formulaire déprécié était toujours là… volontairement car même déprécié, cela permet de ne pas casser ceux qui l'utilisaient à part. Je ne sais pas hein, mais faudrait déjà demander si c'était voulu.

Bah c'est le principe d'une PR, on accepte ou pas, en fonction de ce qu'on sait du code. Mais oui oki pour faire dans une PR à part (c'est juste qu'avec ce formulaire qui restait encore, j'ai mis un bout de temps à comprendre que je testais pas le bon fichier ! Donc pour moi c'était clairement lié, mais ok).

> Peut-être que le formulaire déprécié était toujours là… volontairement car même déprécié, cela permet de ne pas casser ceux qui l'utilisaient à part. Je ne sais pas hein, mais faudrait déjà demander si c'était voulu. Bah c'est le principe d'une PR, on accepte ou pas, en fonction de ce qu'on sait du code. Mais oui oki pour faire dans une PR à part (c'est juste qu'avec ce formulaire qui restait encore, j'ai mis un bout de temps à comprendre que je testais pas le bon fichier ! Donc pour moi c'était clairement lié, mais ok).
maieul force-pushed issue4872_perte_lien from fafca07e4a to 161e0dab18 3 months ago
tcharlss approved these changes 3 months ago
tcharlss left a comment
Owner

testé et approuvé !

testé et approuvé !
Owner

Pour info, @marcimat propose un patch qui règle le bug dans bigup par ici spip/bigup#4862

Pour info, @marcimat propose un patch qui règle le bug dans bigup par ici https://git.spip.net/spip/bigup/pulls/4862
Owner

oui parce que pour moi ce patch ici est pas acceptable : peut-être que ça bug plus avec bigup mais ça ne marcherait plus correctement sans bigup où l'on post tout en une fois, et du coup ça ignorerait totalement une éventuelle modif de parents.

Bref, on peut fermer ici
(et éventuellement supprimer le vieux formulaire joindre sur la version dev mais sans reporter)

oui parce que pour moi ce patch ici est pas acceptable : peut-être que ça bug plus avec bigup mais ça ne marcherait plus correctement sans bigup où l'on post tout en une fois, et du coup ça ignorerait totalement une éventuelle modif de parents. Bref, on peut fermer ici (et éventuellement supprimer le vieux formulaire joindre sur la version dev mais sans reporter)
maieul closed this pull request 3 months ago
Poster
Collaborator

@cerdic

faux :) J'ai fait les tests aussi sans bigup. La raison pour laquelle c'est faux ? L'effacement des infos sur les parents en request n'a lieu que si la condition suivante est remplie

       if(
                _request('copier_local')
                or _request('joindre_upload')
                or _request('joindre_ftp')
                or _request('joindre_distant')
                or _request('joindre_zip')
        )

Ce qui n'arrive pas si on veut modifier les parents, puisqu'on clique alors sur le bouton du bas du formulaire, et non pas celui associé à la modif du document. Et donc on ne post pas joindre_upload ou autre.

Cela étant le bug n'avait pas lieu en l'absence de bigup. Donc ok pour fermer :)

@cerdic faux :) J'ai fait les tests aussi sans bigup. La raison pour laquelle c'est faux ? L'effacement des infos sur les parents en `request` n'a lieu que si la condition suivante est remplie ```` if( _request('copier_local') or _request('joindre_upload') or _request('joindre_ftp') or _request('joindre_distant') or _request('joindre_zip') ) ```` Ce qui n'arrive pas si on veut modifier les parents, puisqu'on clique alors sur le bouton du bas du formulaire, et non pas celui associé à la modif du document. Et donc on ne post pas `joindre_upload` ou autre. Cela étant le bug n'avait pas lieu en l'absence de bigup. Donc ok pour fermer :)

Reviewers

tcharlss approved these changes 3 months ago
Please reopen this pull request to perform a merge.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
5 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.