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

Preserve image attributes when cropping a custom logo image #2441

Closed
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
66f9cc5
Copy attachment's description, title, and caption over to the cropped…
anton-vlasenko Mar 16, 2022
57c95ae
Improve detection of empty strings.
anton-vlasenko Mar 17, 2022
eaadcd3
1. Rename $attachmenet to $original_attachement.
anton-vlasenko Mar 17, 2022
0701c5a
Don't call get_post_meta to retrieve _wp_attachment_image_alt option.
anton-vlasenko Mar 17, 2022
3245151
1. Don't call wp_basename if the image has custom title.
anton-vlasenko Mar 17, 2022
4c5f314
Add test template.
anton-vlasenko Mar 17, 2022
37c0724
Implement test.
anton-vlasenko Mar 18, 2022
1007a0c
1. Fix code style.
anton-vlasenko Mar 21, 2022
28b56b4
Fix the test: properly delete cropped images.
anton-vlasenko Mar 21, 2022
24128be
Remove unused variables.
anton-vlasenko Mar 22, 2022
2493ce7
Copy the post title attribute from original image.
anton-vlasenko Mar 22, 2022
4209559
Fix covers tags.
anton-vlasenko Mar 22, 2022
bb8b3f9
Fix PHPDOC description.
anton-vlasenko Mar 22, 2022
fdaa748
Don't use Yoda conditions for comparisons.
anton-vlasenko Mar 22, 2022
4a54866
Update PHPDOC block.
anton-vlasenko Mar 22, 2022
f96f9d3
Add fail messages to assertions.
anton-vlasenko Mar 22, 2022
c55674c
Second attempt to implement tear_down method.
anton-vlasenko Mar 22, 2022
854fe54
Refactoring: move $this->_setRole( 'administrator' ); to the set_up m…
anton-vlasenko Mar 22, 2022
a4cb089
Core doesn't use @return void tags.
anton-vlasenko Mar 22, 2022
412e862
Refactor @covers tag.
anton-vlasenko Mar 22, 2022
743688e
The class name should follow this convention Tests_[group]_FunctionNa…
anton-vlasenko Mar 22, 2022
a2e8c86
Don't use mb_strlen to detect empty strings.
anton-vlasenko Mar 22, 2022
a39a551
Refactor Tests_Ajax_WpAjaxCropImage::make_attachment and use the _mak…
anton-vlasenko Mar 23, 2022
5d3b37d
Move fixture methods above tests for consistency.
hellofromtonya Mar 23, 2022
de84052
Check if original title was edited
hellofromtonya Mar 23, 2022
0757d9b
Fixed incorrect detection of an "edited" state.
anton-vlasenko Mar 23, 2022
06ecc30
Improve detection of the "edited" state.
anton-vlasenko Mar 23, 2022
8982741
Add assertions for cases when post_title gets populated.
anton-vlasenko Mar 24, 2022
e6261c9
Fix the code style.
anton-vlasenko Mar 24, 2022
9af19ec
Fix code style.
anton-vlasenko Mar 24, 2022
e6be279
Improve image title check comment.
hellofromtonya Mar 24, 2022
d8a1643
Wrap conditional for consistency.
hellofromtonya Mar 24, 2022
ca8fe3a
Formatting - remove empty line.
hellofromtonya Mar 24, 2022
9e2a262
Don't call update_post_meta because the _wp_attachment_image_alt attr…
anton-vlasenko Mar 25, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 29 additions & 4 deletions src/wp-admin/includes/ajax-actions.php
Expand Up @@ -3970,23 +3970,48 @@ function wp_ajax_crop_image() {
/** This filter is documented in wp-admin/includes/class-custom-image-header.php */
$cropped = apply_filters( 'wp_create_file_in_uploads', $cropped, $attachment_id ); // For replication.

$parent_url = wp_get_attachment_url( $attachment_id );
$url = str_replace( wp_basename( $parent_url ), wp_basename( $cropped ), $parent_url );
$parent_url = wp_get_attachment_url( $attachment_id );
$parent_basename = wp_basename( $parent_url );
$url = str_replace( $parent_basename, wp_basename( $cropped ), $parent_url );

$size = wp_getimagesize( $cropped );
$image_type = ( $size ) ? $size['mime'] : 'image/jpeg';

// Get the original image's post to pre-populate the cropped image.
$original_attachment = get_post( $attachment_id );
$sanitized_post_title = sanitize_file_name( $original_attachment->post_title );
$use_original_title = (
( '' !== trim( $original_attachment->post_title ) ) &&
/*
* Check if the original image has a title other than the "filename" default,
* meaning the image had a title when originally uploaded or its title was edited.
*/
( $parent_basename !== $sanitized_post_title ) &&
( pathinfo( $parent_basename, PATHINFO_FILENAME ) !== $sanitized_post_title )
);
$use_original_description = ( '' !== trim( $original_attachment->post_content ) );
hellofromtonya marked this conversation as resolved.
Show resolved Hide resolved

$object = array(
'post_title' => wp_basename( $cropped ),
'post_content' => $url,
'post_title' => $use_original_title ? $original_attachment->post_title : wp_basename( $cropped ),
'post_content' => $use_original_description ? $original_attachment->post_content : $url,
'post_mime_type' => $image_type,
'guid' => $url,
'context' => $context,
);

// Copy the image caption attribute (post_excerpt field) from the original image.
if ( '' !== trim( $original_attachment->post_excerpt ) ) {
$object['post_excerpt'] = $original_attachment->post_excerpt;
}
Comment on lines +4002 to +4005
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of a nit-pick, but I think it would make sense to include this immediately in the associative array above, right after post_title and post_content, which would clarify the relationship of the three when reading the code. The default for this would need to be the empty string (''), which is the default for wp_insert_post (and thus wp_insert_attachment).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback.
I intentionally didn't include it in the associative array.
Why? I prefer not to specify a default value if the task does not require that.
I prefer to let wp_insert_attachment decide what value to use as default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point. Works for me 👍


$attachment_id = wp_insert_attachment( $object, $cropped );
$metadata = wp_generate_attachment_metadata( $attachment_id, $cropped );

// Copy the image alt text attribute from the original image.
if ( '' !== trim( $original_attachment->_wp_attachment_image_alt ) ) {
update_post_meta( $attachment_id, '_wp_attachment_image_alt', wp_slash( $original_attachment->_wp_attachment_image_alt ) );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to my above comment, we could integrate this into the associative array passed to wp_insert_attachment above, which would colocate it with the other other related data. Other than in my previous comment though, here it would be more about avoiding an extra call to update_post_meta.

We can use the meta_input key (supported by wp_insert_post and thus also by wp_insert_attachment) to pass this information, basically like $object['meta_input'] = array( '_wp_attachment_image_alt' => wp_slash( $original_attachment->_wp_attachment_image_alt ) ).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I absolutely agree. I didn't know wp_insert_post supports the meta_input key.
Fixed in 9e2a262
cc @hellofromtonya
I've implemented the change suggested in @felixarntz`s comment above. I tested it manually and it worked as intended. Unit tests also passed.
But if that change will require retesting all use cases manually, please let me know, and I will revert that change.
I also don't want to block this PR from getting merged.


/**
* Filters the cropped image attachment metadata.
*
Expand Down
223 changes: 223 additions & 0 deletions tests/phpunit/tests/ajax/wpAjaxCropImage.php
@@ -0,0 +1,223 @@
<?php

/**
* Admin Ajax functions to be tested.
*/
require_once ABSPATH . 'wp-admin/includes/ajax-actions.php';

hellofromtonya marked this conversation as resolved.
Show resolved Hide resolved
require_once ABSPATH . 'wp-admin/includes/class-wp-filesystem-base.php';
require_once ABSPATH . 'wp-admin/includes/class-wp-filesystem-direct.php';

/**
* Class for testing ajax crop image functionality.
*
* @group ajax
* @covers ::wp_ajax_crop_image
*/
class Tests_Ajax_WpAjaxCropImage extends WP_Ajax_UnitTestCase {

/**
* @var WP_Post|null
*/
private $attachment;

/**
* @var WP_Post|null
*/
private $cropped_attachment;

public function set_up() {
parent::set_up();

// Become an administrator.
$this->_setRole( 'administrator' );
}

public function tear_down() {
if ( $this->attachment instanceof WP_Post ) {
wp_delete_attachment( $this->attachment->ID, true );
}

if ( $this->cropped_attachment instanceof WP_Post ) {
wp_delete_attachment( $this->cropped_attachment->ID, true );
}
$this->attachment = null;
$this->cropped_attachment = null;

parent::tear_down();
}

/**
* Tests that attachment properties are copied over to the cropped image.
*
* @ticket 37750
*/
public function test_it_copies_metadata_from_original_image() {
$this->attachment = $this->make_attachment( true );
$this->prepare_post( $this->attachment );

// Make the request.
try {
$this->_handleAjax( 'crop-image' );
} catch ( WPAjaxDieContinueException $e ) {
}

$response = json_decode( $this->_last_response, true );
$this->validate_response( $response );

$this->cropped_attachment = get_post( $response['data']['id'] );
$this->assertInstanceOf( WP_Post::class, $this->cropped_attachment, 'get_post function must return an instance of WP_Post class' );
$this->assertNotEmpty( $this->attachment->post_title, 'post_title value must not be empty for testing purposes' );
$this->assertNotEmpty( $this->cropped_attachment->post_title, 'post_title value must not be empty for testing purposes' );
$this->assertSame( $this->attachment->post_title, $this->cropped_attachment->post_title, 'post_title value should be copied over to the cropped attachment' );
$this->assertSame( $this->attachment->post_content, $this->cropped_attachment->post_content, 'post_content value should be copied over to the cropped attachment' );
$this->assertSame( $this->attachment->post_excerpt, $this->cropped_attachment->post_excerpt, 'post_excerpt value should be copied over to the cropped attachment' );
$this->assertSame( $this->attachment->_wp_attachment_image_alt, $this->cropped_attachment->_wp_attachment_image_alt, '_wp_attachment_image_alt value should be copied over to the cropped attachment' );
}

/**
* Tests that post_title gets populated if it wasn't modified.
*
* @ticket 37750
*/
public function test_it_populates_title_if_title_was_not_modified() {

$this->attachment = $this->make_attachment( true );
$filename = $this->get_attachment_filename( $this->attachment );
$this->attachment = get_post(
wp_update_post(
array(
'ID' => $this->attachment->ID,
'post_title' => $filename,
)
)
);

$this->prepare_post( $this->attachment );

// Make the request.
try {
$this->_handleAjax( 'crop-image' );
} catch ( WPAjaxDieContinueException $e ) {
}

$response = json_decode( $this->_last_response, true );
$this->validate_response( $response );

$this->cropped_attachment = get_post( $response['data']['id'] );
$this->assertInstanceOf( WP_Post::class, $this->cropped_attachment, 'get_post function must return an instance of WP_Post class' );
$this->assertStringStartsWith( 'cropped-', $this->cropped_attachment->post_title, 'post_title attribute should start with "cropped-" prefix, i.e. it has to be populated' );
}

/**
* Tests that attachment properties get populated if they are not defined (but specific logic depends on the actual property).
*
* @ticket 37750
*/
public function test_it_doesnt_generate_new_metadata_if_metadata_is_empty() {
$this->attachment = $this->make_attachment( false );
$this->prepare_post( $this->attachment );

// Make the request.
try {
$this->_handleAjax( 'crop-image' );
} catch ( WPAjaxDieContinueException $e ) {
}

$response = json_decode( $this->_last_response, true );
$this->validate_response( $response );

$this->cropped_attachment = get_post( $response['data']['id'] );
$this->assertInstanceOf( WP_Post::class, $this->cropped_attachment, 'get_post function must return an instance of WP_Post class' );
$this->assertEmpty( $this->attachment->post_title, 'post_title value must be empty for testing purposes' );
$this->assertNotEmpty( $this->cropped_attachment->post_title, 'post_title value must be auto-generated if it\'s empty in the original attachment' );
$this->assertSame( $this->get_attachment_filename( $this->cropped_attachment ), $this->cropped_attachment->post_title, 'post_title attribute should contain filename of the cropped image' );
$this->assertStringStartsWith( 'cropped-', $this->cropped_attachment->post_title, 'post_title attribute should start with "cropped-" prefix, i.e. it has to be populated' );
$this->assertStringStartsWith( 'http', $this->cropped_attachment->post_content, 'post_content value should contain an URL if it\'s empty in the original attachment' );
$this->assertEmpty( $this->cropped_attachment->post_excerpt, 'post_excerpt value must be empty if it\'s empty in the original attachment' );
$this->assertEmpty( $this->cropped_attachment->_wp_attachment_image_alt, '_wp_attachment_image_alt value must be empty if it\'s empty in the original attachment' );
}

/**
* Creates an attachment.
*
* @return WP_Post
*/
private function make_attachment( $with_metadata = true ) {
$uniq_id = uniqid( 'crop-image-ajax-action-test-' );

$test_file = DIR_TESTDATA . '/images/test-image.jpg';
$upload_directory = wp_upload_dir();
$uploaded_file = $upload_directory['path'] . '/' . $uniq_id . '.jpg';
$filesystem = new WP_Filesystem_Direct( true );
$filesystem->copy( $test_file, $uploaded_file );

$attachment_data = array(
'file' => $uploaded_file,
'type' => 'image/jpg',
'url' => 'http://localhost/foo.jpg',
);

$attachment_id = $this->_make_attachment( $attachment_data );
$post_data = array(
'ID' => $attachment_id,
'post_title' => $with_metadata ? 'Title ' . $uniq_id : '',
'post_content' => $with_metadata ? 'Description ' . $uniq_id : '',
'context' => 'custom-logo',
'post_excerpt' => $with_metadata ? 'Caption ' . $uniq_id : '',
);

// Update the post because _make_attachment method doesn't support these arguments.
wp_update_post( $post_data );

if ( $with_metadata ) {
update_post_meta( $attachment_id, '_wp_attachment_image_alt', wp_slash( 'Alt ' . $uniq_id ) );
}

return get_post( $attachment_id );
}

/**
* @param array $response Response to validate.
*/
private function validate_response( $response ) {
$this->assertArrayHasKey( 'success', $response, 'Response array must contain "success" key.' );
$this->assertArrayHasKey( 'data', $response, 'Response array must contain "data" key.' );
$this->assertNotEmpty( $response['data']['id'], 'Response array must contain "ID" value of the post entity.' );
}

/**
* Prepares $_POST for crop-image ajax action.
*
* @param WP_Post $attachment
*/
private function prepare_post( WP_Post $attachment ) {
$_POST = array(
'wp_customize' => 'on',
'nonce' => wp_create_nonce( 'image_editor-' . $attachment->ID ),
'id' => $attachment->ID,
'context' => 'custom_logo',
'cropDetails' =>
array(
'x1' => '0',
'y1' => '0',
'x2' => '100',
'y2' => '100',
'width' => '100',
'height' => '100',
'dst_width' => '100',
'dst_height' => '100',
),
'action' => 'crop-image',
);
}

/**
* @param WP_Post $attachment
*
* @return string
*/
private function get_attachment_filename( WP_Post $attachment ) {
return wp_basename( wp_get_attachment_url( $attachment->ID ) );
}
}