UNIMOODLE / moodle-mod_certifygen

GNU General Public License v3.0
1 stars 1 forks source link

Error en path #58

Closed tmas0 closed 1 month ago

tmas0 commented 1 month ago

Buenas,

En el repo de onedrive está puesto:

$completefilepath = $CFG->dirroot . '/mod/certifygen/repository/onedrive/tempfiles/' . $reportname;

https://github.com/UNIMOODLE/p31_mod/blob/cbee0a0a0522319b0dedde4fa6a011cebb8684fc/repository/onedrive/classes/certifygenrepository_onedrive.php#L88

Y esto es un bug grave, ya que el directorio web de Moodle no se pueden almacenar ficheros. En su caso existe el temp o data, pero uno u otro o ambos.

Un saludo

xpazv commented 1 month ago

En revisión.

xpazv commented 1 month ago

Propuesta de solución: cambio de $CFG->dirroot por $CFG->dataroot

tmas0 commented 1 month ago

Totalmente de acuerdo con ello.

Muchas gracias!

elena3ip commented 1 month ago

Subido a develop

juacas commented 1 month ago

Disculpad la intervención tardía, pero ¿si el fichero es temporal no deberíamos utilizar el API de ficheros temporales? Detrás de un fichero temporal hay una política de expiración y borrado. Creo que es defectuosa la implementación que hay en:

https://github.com/UNIMOODLE/p31_mod/blob/8d9a2a78707500982d289dbaf80def18042df40c/repository/onedrive/classes/certifygenrepository_onedrive.php#L89

La función correcta para crear el fichero temporal es usar:

/**
     * Copy content of file to temporary folder and returns file path
     *
     * @param string $dir name of the temporary directory
     * @param string $fileprefix prefix of temporary file.
     * @return string|bool path of temporary file or false.
     */
    public function copy_content_to_temp($dir = 'files', $fileprefix = 'tempup_')

En lugar de $file->copy_content_to(...) debería usar:

$completefilepath = $file->copy_contentto:temp('certifygen');

El objeto $file representa al fichero almacenado en el sistema de archivos de Moodle destinado a ficheros de duración media-larga. Este almacenamiento será compartido en cluster o NFS de altas prestaciones y coste. El fichero temporal va a la partición tempdir que es otro FS destinado a ficheros de vida corta.

Por lo que veo, ahora el fichero $completefilepath = $CFG->dataroot . '/temp/' . $reportname; no se borrará nunca, engordando moodle_data, pero en cambio sí se borra el fichero almacenado: $file con $file->delete();

¿$file es un fichero temporal desde el principio? ¿El fichero temporal para subir a Onedrive no se debe borrar?¿Lo borra otra parte del proceso?

¿Es correcta mi interpretación?

elena3ip commented 1 month ago

Hola,

La tarea file_temp_cleanup_task es la encargada de borrar ficheros y directorios que haya dentro de la carpeta temporal del directorio definido en la variable $CFG->dataroot siempre y cuando haya transcurrido el tiempo configurado por la variable $CFG->tempdatafoldercleanup. Por tanto tanto no va a ver problema de borrado, pero sí es cierto que es mas elegante meter todos los archivos temporales en un mismo directorio. Así que procedo a hacer el cambio.

elena3ip commented 1 month ago

Ya está el cambio en develop

tmas0 commented 1 month ago

Comprobado.