45910.md

Folks, I didn't get a chance to finish this review. I've got some notes here, and I have a proposed (100% untested) branch at https://github.com/andrewnicols/moodle/commit/MDL-45910-master

Hi Nejc,

Thank you for working on this issue. I realise that i's quite a complex one and has been sitting for some time without much love. We appreciate your time and effort in looking into it.

On the whole the patch looks okay, but I can see that there are some cases which may be missed and will cause errors to be thrown. I also feel that there is a lot of duplication in this function which could easily be removed with a minor refactor and introduction of a new protected or private function to perform the rotation (e.g. {{rotate_image_by_angle}} taking {{string $source, int $angle, int $width, int $height}} and returning an array containing the final width, height, and image data).

I'd suggest that rather than checking a range for {{$exif['Orientation'] > 0 < 9}}, that you instead use something like {{array_key_exists($exif['Orientation'], $rotation)}}. This will ensure that only the valid rotations are checked.

You have introduced a change in behaviour - when an image was already orientated correctly (Orientation = 1), we previously returned {{[false, false]}}. With these changes we now return dimensions.

Whilst I personally prefer that for consistency, it isn't a documented change or clear why such a change is either necessary or beneficial. We can't make these kinds of change to stable APIs as developers may be depending on the current behaviour.

With regards code duplication, I would strongly recommend creating a new function similar to: {code} protected function rotate_image_dimensions(string $source, int $angle, int $width, int $height): ?array { $imagedata = imagecreatefromstring($source);

    if (!$imagedata) {
        imagedestroy($imagedata);
        return null;
    }

    $result = [
        'image' => $imagedata,
        'width' => $width,
        'height' => $height,
    ];

    if ($angle === 180) {
        $result['image'] = imagerotate($result['image'], $angle, 0);
    }

    if ($angle === 90 || $angle === 270) {
        $result['width'] = $height;
        $result['height'] = $width;
        $result['image'] = imagerotate($result['image'], $angle, 0);
    }

    return $result;
}

{code}

I'd also recommend adjusting the rotate_image function to reduce duplication and unnecessary code paths.