WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 13 months ago

Last modified 13 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 14 months ago.
43226-unit-test.diff (2.0 KB) - added by chetan200891 13 months ago.
Unit test.
43226.2.diff (2.6 KB) - added by birgire 13 months ago.

Download all attachments as: .zip

Change History (23)

#1 @leemon
14 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
14 months ago

  • Keywords dev-feedback added

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


14 months ago

#4 @SergeyBiryukov
14 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
14 months ago

  • Version changed from 4.9.2 to 4.7

Introduced in [38949].

@chetan200891
14 months ago

@chetan200891
13 months ago

Unit test.

#6 @chetan200891
13 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.


13 months ago

#8 @audrasjb
13 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.


13 months ago

#10 @mikeschroder
13 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.


13 months ago

#12 @audrasjb
13 months ago

  • Keywords 2nd-opinion added

#13 @audrasjb
13 months ago

  • Keywords dev-feedback added

@birgire
13 months ago

#14 @birgire
13 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
13 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
13 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.


13 months ago

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


13 months ago

#19 @mikeschroder
13 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
13 months ago

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