Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref #429 -- fixed server location to build-in filter fix #1320

Merged
merged 5 commits into from
Feb 10, 2017

Conversation

jarednova
Copy link
Member

Ticket: #429

Issue

#429 and #1131 describe issues where images stored on Amazon S3 do not load from S3 when Timber is activated

Solution

Apply a widely-used patch directly to Timber\Image:

// Add this to functions.php
function fix_timber_s3_src( $src, $ID ) {
    $size = '';
    $attachment_src = wp_get_attachment_image_src($ID, $size)[0];
    $fixed_src = $attachment_src ? $attachment_src : $src;
    return $fixed_src;
}
add_filter( 'timber_image_src', 'fix_timber_s3_src', 10, 2 );

Impact

Makes is cleaner! But also could have fallout based on what's being over-written

Usage

None

Considerations

This does NOT solve the resizing issues described in #429

Testing

Tests pass locally, no new tests though

@coveralls
Copy link

coveralls commented Feb 10, 2017

Coverage Status

Coverage decreased (-0.004%) to 97.368% when pulling 7d9182a on 429/off_server_location into 7a6749a on master.

@codecov-io
Copy link

codecov-io commented Feb 10, 2017

Codecov Report

Merging #1320 into master will not change coverage.

@@            Coverage Diff            @@
##             master    #1320   +/-   ##
=========================================
  Coverage     97.64%   97.64%           
+ Complexity     1403     1400    -3     
=========================================
  Files            45       45           
  Lines          3276     3276           
=========================================
  Hits           3199     3199           
  Misses           77       77
Impacted Files Coverage Δ Complexity Δ
lib/Image.php 100% <100%> (ø) 66 <ø> (-6)
lib/MenuItem.php 100% <ø> (ø) 46% <ø> (+3%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a6749a...34a5ab3. Read the comment docs.

@coveralls
Copy link

coveralls commented Feb 10, 2017

Coverage Status

Coverage increased (+0.003%) to 97.374% when pulling 20033ff on 429/off_server_location into 7a6749a on master.

@jarednova
Copy link
Member Author

@tvanantwerp can you review and see if this solves the issues for you in #429?

@tvanantwerp
Copy link

Output of post.thumbnail.src and sized variants exactly matches the expected S3 URLs with these changes.

I have no idea how the line deletions in the src function will affect any other Timber\Image or Timber\ImageHelper functions, if at all--but I pretty much only ever used post.thumbnail.src and wouldn't notice any potential side effects.


Not directly related to #429 but when I tested post.thumbnail.width or post.thumbnail.height, it returns NULL. Not something I really need or is within the scope of that issue and the bounty, but thought it worth mentioning. I've used {{function('getimagesize', post.thumbnail.src)[0]}} as a workaround in rare cases when I want that info. I know that wp_get_attachment_image_src also returns the image dimensions, so I imagine width and height could be fixed similarly if it's not just me experiencing the problem.

@jarednova
Copy link
Member Author

Great, thanks @tvanantwerp! I figure while we're here I'd love to take care of the width/height stuff too, so I'll create a fresh issue + PR to work from on that front

@jarednova jarednova merged commit 3818a31 into master Feb 10, 2017
@jarednova jarednova deleted the 429/off_server_location branch February 10, 2017 19:26
@LeighBicknell
Copy link

This merge results in a php >=5.4 requirement that did not previously exist in the codebase.

<php5.4 now throws

PHP Parse error: syntax error, unexpected '[' in /home/www.postalmuseum.org/the-postal-museum-2016/wordpress/wp-content/plugins/timber-library/lib/Image.php on line 444

Due to

$src = wp_get_attachment_image_src($this->ID, $size)[0];

@jarednova
Copy link
Member Author

@davidmaneuver thanks for the heads-up on that; @gchtr submitted a fix in #1330 that I'll be releasing later today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants