WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#43226 closed defect (bug) (fixed)

Crop setting in thumbnails never set when uploading PDF files

Reported by: leemon Owned by: mikeschroder
Milestone: 4.9.5 Priority: normal
Severity: normal Version: 4.7
Component: Media Keywords:
Focuses: administration Cc:

Description

I don't know if it's specific to my server config but, for some reason, the crop setting in thumbnails is never set when I upload PDF files.

Apparently, the following conditional statement in the /wp-admin/includes/image.php file is never true when it's supposed to run for medium and large thumbnails:

https://core.trac.wordpress.org/browser/tags/4.9/src/wp-admin/includes/image.php#L244

If I remove the conditional statement the crop setting is set for all thumbnails.

I'm using WordPress 4.9.2 with PHP 7.0.27

Attachments (3)

43226.diff (537 bytes) - added by chetan200891 4 months ago.
43226-unit-test.diff (2.0 KB) - added by chetan200891 3 months ago.
Unit test.
43226.2.diff (2.6 KB) - added by birgire 3 months ago.

Download all attachments as: .zip

Change History (23)

#1 @leemon
4 months ago

If I change the conditional statement to:

if ( 'thumbnail' !== $s ) {
    $sizes[ $s ]['crop'] = get_option( "{$s}_crop" );
}

it works as expected.

#2 @leemon
4 months ago

  • Keywords dev-feedback added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


4 months ago

#4 @SergeyBiryukov
4 months ago

  • Keywords needs-patch needs-unit-tests added; dev-feedback removed
  • Milestone changed from Awaiting Review to 4.9.5

Hi @leemon, welcome to WordPress Trac! Thanks for the report.

Yes, the current ! 'thumbnail' === $s condition doesn't appear to work as expected. It's only satisfied if $s is boolean false. It should indeed be 'thumbnail' !== $s.

#5 @SergeyBiryukov
4 months ago

  • Version changed from 4.9.2 to 4.7

Introduced in [38949].

@chetan200891
4 months ago

@chetan200891
3 months ago

Unit test.

#6 @chetan200891
3 months ago

Attached Unit Test patch 43226-unit-test.diff

The smallest thumbnail size was specifically excluded from forced cropping, but that the patch would fix it so that the other sizes would honor the forced cropping.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


3 months ago

#8 @audrasjb
3 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


3 months ago

#10 @mikeschroder
3 months ago

  • Owner set to mikeschroder
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 months ago

#12 @audrasjb
3 months ago

  • Keywords 2nd-opinion added

#13 @audrasjb
3 months ago

  • Keywords dev-feedback added

@birgire
3 months ago

#14 @birgire
3 months ago

I tested 43226.diff and 43226-unit-test.diff but the unit-test failed.

It ran successfully when I changed:

'medium'    => array(
	'file'      => 'wordpress-gsoc-flyer-pdf-232x300.jpg',
	'width'     => 300,
	'height'    => 300,
	'mime-type' => 'image/jpeg',
),

to:

'medium'    => array(
	'file'      => 'wordpress-gsoc-flyer-pdf-300x300.jpg',
	'width'     => 300,
	'height'    => 300,
	'mime-type' => 'image/jpeg',
),

Ticket #39975 aims to Remove direct calls to '/tmp/' in Unit Tests.

So 43226.2.diff uses get_temp_dir() instead of '/tmp/'.

If we want to use:

$test_file = wp_tempnam( 'wordpress-gsoc-flyer.pdf' );

then we have to deal with files like:

/tmp/wordpress-gsoc-flyer-Y5yoVa.tmp

making it a little more involved to create the $expected array and then to unlink the corresponding files.

#15 @mikeschroder
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 42792:

Media: Correctly allow changing PDF thumbnail crop value.

Corrects logic that keeping plugins from setting crop value of intermediate image sizes for rendered PDFs.

Adds test.

Props leemon, SergeyBiryukov, chetan200891, birgire.
Fixes #43226.

#16 @mikeschroder
3 months ago

  • Keywords fixed-major added; has-patch has-unit-tests 2nd-opinion dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.9.5 consideration

This ticket was mentioned in Slack in #core-committers by audrasjb. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-media by mike. View the logs.


3 months ago

#19 @mikeschroder
3 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 42813:

Media: Correctly allow changing PDF thumbnail crop value.

Corrects logic that kept plugins from setting crop value of intermediate image sizes for rendered PDFs.
Adds test.

Props leemon, SergeyBiryukov, chetan200891, birgire.
Merges [42792] to the 4.9 branch.
Fixes #43226.

#20 @mikeschroder
3 months ago

  • Keywords fixed-major removed
Note: See TracTickets for help on using tickets.