Make WordPress Core

Opened 7 years ago

Closed 2 years ago

Last modified 2 years ago

#39108 closed defect (bug) (fixed)

Media: can't leave an image "untitled" from the "Edit Media" menu

Reported by: dromero20's profile dromero20 Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: normal Version: 4.6.1
Component: Media Keywords: good-first-bug has-patch has-testing-info assigned-for-commit commit add-to-field-guide
Focuses: Cc:

Description

Just found this issue while testing the Media Library. If you try to remove the image's title from the "Edit Media", it gets "autorenamed" with the original name of the uploaded file. You can't leave that image untitled. But, if you rename the image from the "Attachment Details" menu, you can leave the image untitled perfectly.

I've made a gif of the process, I hope it helps to understand what I'm saying:
http://hgrequests.files.wordpress.com/2016/12/media.gif

Steps to reproduce
-Upload an image.
-Go to Media Library (wp-admin)
-Select Mode:List View and click on the image
-Change the name, Update. Then, remove the name, Update again (the image will be renamed with the file's original name)

Attachments (4)

media.gif (4.9 MB) - added by dromero20 7 years ago.
Gif of the issue
Jan-05-2018 08-41-38.mp4 (6.9 MB) - added by desrosj 6 years ago.
Updated demo of the bug
39108.patch (1.2 KB) - added by Junaidkbr 6 years ago.
39108.diff (2.0 KB) - added by etaproducto 2 years ago.

Change History (35)

@dromero20
7 years ago

Gif of the issue

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


6 years ago

#2 @desrosj
6 years ago

  • Keywords needs-patch good-first-bug added

Hi @dromero20, and welcome to Trac!

I was able to reproduce this bug. Are you interested in creating a patch for this? For help with that, you can check out the handbook: https://make.wordpress.org/core/handbook/contribute/

@desrosj
6 years ago

Updated demo of the bug

@Junaidkbr
6 years ago

#3 @Junaidkbr
6 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

@desrosj My patch takes care of the issue but I had trouble sending the auto generated title back to the media modal.

My patch has changes in https://core.trac.wordpress.org/browser/tags/4.9/src/wp-admin/includes/ajax-actions.php#L2516 where I am returning wp_send_json_success( $json_data ); with $json_data = array( 'title' => $new_title );. But I have no clue how to get it from the AJAX response at https://core.trac.wordpress.org/browser/tags/4.9/src/wp-includes/js/media-views.js?rev=42198#L6331

#4 @DrewAPicture
6 years ago

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

Assigning to mark the good-first-bug as "claimed".

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


3 years ago

#6 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.8

#7 @Boniu91
3 years ago

  • Keywords has-testing-info added

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 years ago

#9 @francina
3 years ago

Is the expected behavior to get the file to be Untitled in both cases or to be renamed in both cases?
Thanks

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


3 years ago

#11 @antpb
3 years ago

It was discussed in the recent Media meeting that we should maybe mimic the behavior of a no-title post. (no title) when no title is set.

This should fit in nicely with the proposed patch but instead using the string posts use for no-titles.

#12 @audrasjb
3 years ago

  • Milestone changed from 5.8 to 5.9

It looks like this ticket still needs some work.
Given we're about to release 5.8 beta 1, let's move it for 5.9 consideration.

#13 @aadilali
3 years ago

WordPress set image name as the default post title if it is empty or post blank. Following function written in "wp-admin\includes\media.php" handle this scenario.

"function image_attachment_fields_to_save()"

You can remove this function to get the (no title) by default.

#14 @sabernhardt
2 years ago

  • Milestone changed from 5.9 to 6.0

#15 @etaproducto
2 years ago

Hi all, first time contributor (maybe?)

Per @aadilali's comment... please see the attached 39108.diff, is it as simple as removing the filter function, it doesn't look (to me) to be needed by anything else?

@etaproducto
2 years ago

#16 follow-up: @azouamauriac
2 years ago

I've tested @etaproducto patch and it works...

Last edited 2 years ago by azouamauriac (previous) (diff)

This ticket was mentioned in PR #2235 on WordPress/wordpress-develop by pdemaria.


2 years ago
#17

Per @aadilali's comment... please see the attached 39108.diff, is it as simple as removing the filter function, it doesn't look (to me) to be needed by anything else?

Trac ticket: #39108

#18 in reply to: ↑ 16 @etaproducto
2 years ago

Replying to azouamauriac:

I've tested @etaproducto patch and its works...

Is that it then, will it be reviewed now? Saw that you could make a fork of WordPress/wordpress-develop and create a pull request there as well... (I made a try of doing it with my cloned repo, like a noob... so had to close that)

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


2 years ago

#20 @Boniu91
2 years ago

Works as expected for me.

Test Report

Env

  • WordPress 6.0-alpha-52448-src

Steps to test

  1. Go to the media library grid View
  2. Edit one media, erase the title and save settings
  3. Switch to the list view - the image has no title
  4. Edit image from the list view, make sure that the title is empty and save settings
  5. In the list view - the image still has no title

#21 @audrasjb
2 years ago

  • Keywords assigned-for-commit added; needs-testing removed
  • Owner changed from junaidkbr to audrasjb
  • Status changed from assigned to reviewing

Thanks for the test @Boniu91! Self assigning for commit consideration.

#22 @audrasjb
2 years ago

  • Keywords needs-dev-note added

Adding needs-dev-note since this change deserves a mention in the Media (or Misc changes) dev note to document that we removed this filter.

#23 follow-up: @audrasjb
2 years ago

Instead of completely remove this filter, shouldn't we deprecate it?

Looks like at least 64 plugins are using it: https://wpdirectory.net/search/01FZJMWDNYAJPXRGH546GQ8C24

Pinging @SergeyBiryukov for experienced advice on the best way to handle this.

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


2 years ago

#25 in reply to: ↑ 23 @SergeyBiryukov
2 years ago

Replying to audrasjb:

Instead of completely remove this filter, shouldn't we deprecate it?

Looks like at least 64 plugins are using it: https://wpdirectory.net/search/01FZJMWDNYAJPXRGH546GQ8C24

It looks like most of those are either false positives or copies of the original function, though removing the function would indeed cause backward compatibility concerns. If it's no longer relevant, it should be deprecated instead.

I was also curious whether new attachments would still get their title on upload if this filter is removed. Upon some testing, media_handle_upload() still assigns the initial title as expected.

Some history here:

  • media_handle_upload() was introduced in [6659].
  • image_attachment_fields_to_save() was introduced in [6876] / #5874.

To summarize, removing this line is enough to fix the issue and achieve the desired result of using the (no title) fallback, as mentioned in comment:11:

add_filter( 'attachment_fields_to_save', 'image_attachment_fields_to_save', 10, 2 );

Whether the function itself should be deprecated can be up for discussion. If it's not used anywhere else in core, I think it makes sense to deprecate it.

#27 @audrasjb
2 years ago

Makes sense to me as well.
@SergeyBiryukov when you have a chance, I would be happy to get a quick double check on the above PR. It removes the filter and it also deprecates the function.

#28 @audrasjb
2 years ago

  • Keywords commit added

Alright, I double checked the current PR and I think it's good to go.

#29 @audrasjb
2 years ago

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

In 53228:

Media: Remove attachment_fields_to_save filter and deprecate image_attachment_fields_to_save().

This filter prevented removing attachment titles. This changeset removes the filter and deprecates the related function since it is no longer used.

Props dromero20, desrosj, Junaidkbr, francina, antpb, audrasjb, aadilali, etaproducto, azouamauriac, Boniu91, SergeyBiryukov.
Fixes #39108.

#31 @milana_cap
2 years ago

  • Keywords add-to-field-guide added; needs-dev-note removed

We have new keyword for tickets that don't need dedicated dev note but rather just mentioning in Field guide or misc dev note.

Note: See TracTickets for help on using tickets.