catalyst / mahara-module_objectfs

Object file system for Mahara.
GNU General Public License v3.0
3 stars 2 forks source link

Issue #17: Pass data (size) array to get_local_path() #18

Closed djarran closed 1 month ago

djarran commented 2 months ago

Description:

This PR resolves Issue #17 by updating get_path() and get_remote_path() to accept an array that contains resize data.

golenkovm commented 2 months ago

Thanks @djarran This seems to be working well in UAT. Let's give it a longer test (eg a week or something) and merge if nothing explodes. Cheers

djarran commented 2 months ago

The original patch for this PR only worked if the image was already local (i.e. recently uploaded or pulled from the cloud storage service).

This second patch checks if the data passed to get_remote_path (here) contains resize parameters*. If it does, We call get_local_path($data, true) (here). Passing the true argument ensures that the image is local, see below

public function get_local_path($data=array(), $generateifpossible = true) {
    require_once('file.php');
    $result = get_dataroot_image_path('artefact/file/', $this->fileid, $data, $this->orientation, $generateifpossible);
    return $result;
}

function get_dataroot_image_path($path, $id, $size=null, $orientation = 0, $generateifpossible = true) {
    ...
    if ($generateifpossible) {
        // We have to have an original image locally to be able to build preview,
        // because image functions like getimagesize do not work with stream wrappers
        // So if the original image is not readable, try to download it from an external system (if enabled).
        if (is_using_external_filesystem()) {
            $image = artefact_instance_from_id($id);
-------->   $image->ensure_local();
        }
        ...
    }

public function ensure_local($fileartefact) {

    if (empty($fileartefact->get('contenthash'))) {
        // If theres no contenthash then we dont have an external path for this object
        return;
    }

    $status = $this->get_file_location_status($fileartefact);

    if ($status == OBJECT_LOCATION_EXTERNAL) {

        $this->acquire_object_lock($fileartefact);

        $location = $this->copy_object_from_external_to_local($fileartefact, $fileartefact->get('size'));

        $this->release_object_lock($fileartefact);

        update_object_record($fileartefact, $location);
    }
}

The way this is implemented avoids a previous bug in Mahara that caused an infinite loop

*Currently the only data that would be passed in the $data array are resize parameters, so we may only need to check if $data is not empty in get_remote_path(), but this does futureproof the implementation and make it more explicit)

golenkovm commented 1 month ago

No issues reported during one week in Production. Merging this in :+1: