Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

BUG #2553 Fixed how HTMLEditorField calls ResizedImage function #2564

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

thezenmonkey commented Oct 18, 2013

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.

@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
Owner

chillu commented Oct 21, 2013

@simonwelsh @ajshort Any chance you could peer review this? I think I've looked at it for too long now, and lost my good judgement ;) Its a gnarly issue - context here: #2553

@simonwelsh simonwelsh and 1 other commented on an outdated diff Oct 21, 2013

forms/HtmlEditorField.php
}
+
+ // if there is a source image resize it and check if it resized image exists
+ if($resize) $resized=$resize->ResizedImage($width, $height);
@simonwelsh

simonwelsh Oct 21, 2013

Contributor

This is causing Travis to fail.

@thezenmonkey

thezenmonkey Oct 21, 2013

Contributor

Okay I think I see the problem $resize should be changed to $source thorughout. Do I submit a new PR?

@simonwelsh

simonwelsh Oct 21, 2013

Contributor

Amend your commit and push again.

@thezenmonkey thezenmonkey Fixed Undeclared variable issues
Changed $resize to $source and decalred $resized to address undeclared variables
908790b
Contributor

thezenmonkey commented Oct 22, 2013

@simonwelsh, I'm not sure why the second PR is failing behat user password.

Owner

chillu commented Dec 13, 2013

Hmm, given the complexity of the issue here, I'd really love to see some tests. Richard, would you be able to write some? You can base them off HtmlEditorFieldTest->testImageInsertion(). FileTest->setUp() has an example on how to rely on file fixtures.

Contributor

thezenmonkey commented Dec 13, 2013

I'll take a look. I'm fairly new to testing but I'll give it a try

Sent from my iPhone

On Dec 13, 2013, at 6:57 AM, Ingo Schommer notifications@github.com wrote:

Hmm, given the complexity of the issue here, I'd really love to see some tests. Richard, would you be able to write some? You can base them off HtmlEditorFieldTest->testImageInsertion(). FileTest->setUp() has an example on how to rely on file fixtures.


Reply to this email directly or view it on GitHub.

Contributor

simonwelsh commented Mar 15, 2014

@thezenmonkey Any luck with those tests?

@simonwelsh simonwelsh added the 3.1 label Mar 15, 2014

Owner

dhensby commented Feb 28, 2015

@chillu is this still a relevant problem? if it is, I think it'd be worth someone writing some tests for it to get this in.

Member

micmania1 commented Mar 2, 2015

This would be much better added as a shortcode. The reason being, if a folder moves (or is renamed) then this path is static and would need to be rewritten which is expensive when that needs to be looked for on every page.

If we just used a shortcode, then we could lookup the file by ID and then handle the resampling and wouldn't need to do any rewrite checks.

Its kind of related to silverstripe/silverstripe-cms#1115

Contributor

tractorcow commented Nov 27, 2015

In 4.x we are are going to shortcode all images in any case. Maybe we could still fix this for 3.2, but in master we've already refactored a lot of this code anyway. This code now exists in Image.php, and will be updated in the coming months to use the planned tuple-shortcode. At the moment it's using a placeholder data-fileid data property.

See

public static function regenerate_html_links($value) {

If you still want to fix this for 3.2, then feel free, although you'll have to raise a new PR on that branch. I'll have to close this pull request, since it's been sitting here for so long without attention.

@tractorcow tractorcow closed this Nov 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment