Preserve image attributes when cropping a custom logo image #2441
Conversation
… image (if they are set).
2. Copy image alt attribute as well.
2. Add comments.
This is WIP.
2. Fix the test. Delete cropeed image.
…me, which changes this test class to Tests_Ajax_WpAjaxCropImage. And the file should be camelCase of the function name, e.g. wpAjaxCropImage.php.
…e_attachment method to create a test post.
| $use_original_title = ( | ||
| '' !== trim( $original_attachment->post_title ) && | ||
| // Check if the original image's title was edited. | ||
| pathinfo( $parent_basename, PATHINFO_FILENAME ) !== $original_attachment->post_title |
There was a problem hiding this comment.
@anton-vlasenko I added this check to only use the original image's title if it was edited. When there's no title in the image's metadata, by default the image's filename is set as the post_title. This check is making sure the post_title is not the same as that default behavior. If no, then it will use the original image's title.
There was a problem hiding this comment.
The tests will need updating though.
There was a problem hiding this comment.
Thank you!
I'm working on updating the tests.
We have to call sanitize_file_name to escape $original_attachment->post_title, otherwise this comparison will not work as intended. E.g. post_title can have spaces, but they get replaced with dashes in the filename. Also, sometimes post_title contains file extension, sometimes it doesn't.
We have to call sanitize_file_name to escape $original_attachment->post_title, otherwise this comparison will not work as intended. E.g. post_title can have spaces, but they get replaced with dashes in the filename.
anton-vlasenko
changed the title
There was a problem hiding this comment.
The implementation and tests LGTM 👍
Approving - pending input from @felixarntz (see context in the Trac ticket comment).
There was a problem hiding this comment.
This looks great @anton-vlasenko! I left two very minor comments for your consideration, although they certainly don't need to block this from getting merged. Please take a look.
@hellofromtonya I've left a comment in https://core.trac.wordpress.org/ticket/37750#comment:33 as well.
| // 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; | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's a fair point. Works for me 👍
| // 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 ) ); | ||
| } |
There was a problem hiding this comment.
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 ) ).
There was a problem hiding this comment.
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.
…ibute can be set with wp_insert_attachment.
anton-vlasenko
force-pushed
the
fix/attachment-properties-are-not-copied-over-to-the-cropped-image
branch
from
d8f5c63
to
9e2a262
Compare
|
Committed via changeset https://core.trac.wordpress.org/changeset/53027. |
This PR aims to implement preserving attributes when cropping a custom logo image.
Steps to test this PR: https://core.trac.wordpress.org/ticket/37750#comment:26
Trac ticket: https://core.trac.wordpress.org/ticket/37750
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.