Skip to content

ResizedImage capitalization regression breaks images on 3.0 -> 3.1 upgrade #2553

Open
chillu opened this Issue Oct 16, 2013 · 6 comments

3 participants

@chillu
SilverStripe Ltd. member
chillu commented Oct 16, 2013

"Upgrading to 3.1 creates issues with images inserted via the TinyMCE. Pre-3.1 the ResizedImage function named the cached files resizedimage{Size}{Filename}. Post 3.1 the ResizedImage function follows the other image functions and names the file {Format}{Size}{Filename} or ResizedImage{Size}{Filename}. Note the capitilization difference. So when a user resaves an object with a TinyMCE it goes through the images and checks that resized versions exist. Unfortuntaly at the filesystem level ResizedImage = resizedimage so the image existis, but the save function still rewrites the src to the new case, which then shows as a broken image link in the browser. "

See https://groups.google.com/forum/#!topic/silverstripe-dev/i0395y1qbKI

This is a regression introduced here:
dd6aaaf#diff-bf03e641675b755b14906a961cf8485fR471

@thezenmonkey

Could this be as symple as wrapping the "Resample the images if the width & height have changed" in file_exists if statement in HTMLEditorField

    public function saveInto(DataObjectInterface $record) {
        if($record->hasField($this->name) && $record->escapeTypeForField($this->name) != 'xml') {
            throw new Exception (
                'HtmlEditorField->saveInto(): This field should save into a HTMLText or HTMLVarchar field.'
            );
        }

        $htmlValue = Injector::inst()->create('HTMLValue', $this->value);

        // Sanitise if requested
        if($this->config()->sanitise_server_side) {
            $santiser = Injector::inst()->create('HtmlEditorSanitiser', HtmlEditorConfig::get_active());
            $santiser->sanitise($htmlValue);
        }

        // Resample images and add default attributes
        if($images = $htmlValue->getElementsByTagName('img')) foreach($images as $img) {
            // strip any ?r=n data from the src attribute
            $realImg = $img->getAttribute('src');
            $img->setAttribute('src', preg_replace('/([^\?]*)\?r=[0-9]+$/i', '$1', $img->getAttribute('src')));

            //Check if a resized file exisits
            if(!file_exists(Director::baseFolder() . '/' .$img->getAttribute('src'))) {
                // Resample the images if the width & height have changed.
                if($image = File::find(urldecode(Director::makeRelative($img->getAttribute('src'))))){
                    $width  = $img->getAttribute('width');
                    $height = $img->getAttribute('height');
                    //$nextImg = $image->Filename;

                    if($width && $height && ($width != $image->getWidth() || $height != $image->getHeight())) {
                        //Make sure that the resized image actually returns an image:
                        $resized=$image->ResizedImage($width, $height);
                        if($resized) $img->setAttribute('src', $resized->getRelativePath());
                    }
                }
            }

            // Add default empty title & alt attributes.
            if(!$img->getAttribute('alt')) $img->setAttribute('alt', '');
            if(!$img->getAttribute('title')) $img->setAttribute('title', '');
        }

        // Store into record
        $record->{$this->name} = $htmlValue->getContent();
    }

That way on it won't do the operation if the resized image already exists?

@chillu
SilverStripe Ltd. member
chillu commented Oct 17, 2013

Unfortuntaly at the filesystem level ResizedImage = resizedimage so the image existis

Its not even checking the filesystem, but rather the DB entry through File::find(), right? And that should be case insensitive. And looking at the if($width... conditional, it only rewrites when the width/height are difference, which would necessitate a new file anyway. And (important!) sets the src attribute of the <img> tag after its done. I tried to reproduce this whole thing by renaming files and paths manually on a 3.1 installation, but then again I'm on a case insensitive filesystem (OSX). Its late, I might be getting myself confused ... could you point out where the faulty comparison occurs exactly?

@thezenmonkey

The if($width in the HTMLEditorField only compares the width and heihgt attributes on the img tag to the width and height of the original file. Then it passes it off to Image to do all the checks for Image_Cached. The problem there is that is use file_exists and on case insensative system like OSX it fire false positives.

Realisitaclly the case issue is only on ResizedImage since all the others have been upper camel case for as long as I can remember. So the question is, is it worth redoing all the file checks in Image to use some sort of case sensitive image search for the sake of this one function, or just head the problem off at the pass and deal with it in the HTMLEditorField since traditionally that's the only place SilverStripe itself used ResizedImage.

Beyond that there should at least be a note in the Upgrade guide that this can happen anywhere a developer has used ResiedImage themselves.

I do realize there is an issue with code I posted (fizes existing the issue wit existin images but creates a problem with ones added), I can keep working on that one since it's probably the quickest way to patch it in.

@thezenmonkey

Okay here is a working solution for upgrade issue. Once again it doesn't actually check the case of the filename, it just makes sure the file in src attribute exists and if its the same as the width and height request.

    public function saveInto(DataObjectInterface $record) {
        if($record->hasField($this->name) && $record->escapeTypeForField($this->name) != 'xml') {
            throw new Exception (
                'HtmlEditorField->saveInto(): This field should save into a HTMLText or HTMLVarchar field.'
            );
        }

        $htmlValue = Injector::inst()->create('HTMLValue', $this->value);

        // Sanitise if requested
        if($this->config()->sanitise_server_side) {
            $santiser = Injector::inst()->create('HtmlEditorSanitiser', HtmlEditorConfig::get_active());
            $santiser->sanitise($htmlValue);
        }

        // Resample images and add default attributes
        if($images = $htmlValue->getElementsByTagName('img')) foreach($images as $img) {
            // strip any ?r=n data from the src attribute
            $img->setAttribute('src', preg_replace('/([^\?]*)\?r=[0-9]+$/i', '$1', $img->getAttribute('src')));

            $width  = $img->getAttribute('width');
            $height = $img->getAttribute('height');

            $srcImage = File::find(urldecode($img->getAttribute('src')));
            $image = File::find(urldecode(Director::makeRelative($img->getAttribute('src'))));


            // check if either the Image in the src exists or a File object with same name exists;
            if($srcImage || $image ) {

                //check if the src and File object are the same
                if($srcImage && $image && ($img->getAttribute('src') == $image->Filename)) {

                    if($width && $height && ($width != $image->getWidth() || $height != $image->getHeight())) {
                        //Make sure that the resized image actually returns an image:
                        $resized=$image->ResizedImage($width, $height);
                        if($resized) $img->setAttribute('src', $resized->getRelativePath());
                    }
                // check if both exist but are not the same 
                } elseif ($srcImage && $image && ($img->getAttribute('src') != $image->Filename)) {

                    // create a temporary new image object with the src as the filename to check the size
                    $newImage = new Image();
                    $newImage->Filename = $img->getAttribute('src');

                    //check if the src image matches the requested file and create a new Cached_Image if needed
                    if($width && $height && ($width != $newImage->getWidth() || $height != $newImage->getHeight())) {
                        $resized=$image->ResizedImage($width, $height);
                        if($resized) $img->setAttribute('src', $resized->getRelativePath());
                    }
                }

            }
            // Add default empty title & alt attributes.
            if(!$img->getAttribute('alt')) $img->setAttribute('alt', '');
            if(!$img->getAttribute('title')) $img->setAttribute('title', '');
        }

        // Store into record
        $record->{$this->name} = $htmlValue->getContent();
    }

I would basically treat this as hot patch so that people upgrading don't break images in HTML fields. A more robust solution would require some major changes to the File class. if an existing lower-camelcase or upper-camelcase img exists and there is no change to the size, it does nothing. If the size differs from the existing file it creates a new one.

@chillu
SilverStripe Ltd. member
chillu commented Oct 18, 2013

Thanks for staying on top of this. Could you raise a pull request so we can peer review more effectively? Can you reduce the duplication a bit? (two ResizedImage() calls). And maybe write a note in the 3.1.0.md upgrade guide, as well as in 3.1.2.md?

@thezenmonkey
@thezenmonkey thezenmonkey added a commit to thezenmonkey/sapphire that referenced this issue Oct 18, 2013
@thezenmonkey thezenmonkey BUG #2553 Fixed how HTMLEditorField calls ResizedImage function to ad…
…dress capitalization issue

Added file check to HTMLEditorField saveInto function so that it would first check for an existing resized image, and compare its size to the the image tag before passing the File off to the Image Object to be resized. This will prevent SilverStripe from regenerating the img tag when the correct resized image already exists.
b4ac350
@simonwelsh simonwelsh added the 3.1 label Mar 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.