Make WordPress Core

Opened 15 months ago

Last modified 7 days ago

#57957 assigned defect (bug)

Media title falsely has kebab case when inserted via REST API (e.g. in Block Editor)

Reported by: abitofmind's profile abitofmind Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.6 Priority: normal
Severity: normal Version: 6.1.1
Component: REST API Keywords: has-patch has-unit-tests reporter-feedback commit
Focuses: rest-api Cc:

Description (last modified by hellofromTonya)

Reproduction I: "Upload New Media" creates media title "My Great Photo"

1) Media > Add New
2) Drag-n-drop "My Great Photo.jpg"
3) Inspecting this in "Edit Media" shows a media title "My Great Photo".

Reproduction II: Insertion via Block Editor (Gutenberg) creates media title "My-Super-Photo"

1) Edit an existing page or create a new page.
2) Set the cursor into an empty block.
3) Drag-n-drop "My Super Photo.jpg" into this block. This automatically creates a media page behind the scenes.
4) Save the page.
5) Inspecting the media title via "Edit Media"

  • Actual: The media title is "My-Super-Photo".
  • Expected: The media title should be "My Super Photo".
    • The media title should be the filename portion (without the suffix) in its purest possible form (UTF-8) and only the slug and filename should then be transformed according settings regarding ascii/whitespace/transliteration/escaping/punycode/etc.
    • Achieving the same goal via different means (Upload New Media vs. drag-n-drop in editor) should result in the same outcome. Otherwise this is an inconsistency.

Analysis & Fix

@adamsilverstein already analyzed and fixed this bug in the Gutenberg issue tracker and asked to re-file here in WordPress Core for review & adding to the codebase.

Adam's Analysis

This is actually something that happens on the core side where the logic to set the title is currently different in the REST API vs. the regular media uploader.

Adam's Fix which should be reviewed

I proposed a fix in WordPress/wordpress-develop #3981 that works in my testing. Would be good to have some additional feedback, maybe from the REST API team.

Environment

Priority

  • If the fix is deemed solid and the review process goes quickly without bumps: Is there a chance that the fix can make it into the 6.2 release? If not, no worry, am happy whenever this will get eventually fixed.

References

Attachments (2)

57957.diff (3.0 KB) - added by adamsilverstein 12 months ago.
57957.2.diff (3.3 KB) - added by adamsilverstein 2 weeks ago.

Download all attachments as: .zip

Change History (43)

#1 @audrasjb
15 months ago

  • Milestone changed from Awaiting Review to 6.2.1

Hello and thanks for the detailed ticket!

This is an annoying bug, but since it wasn't introduced in WP 6.2 development cycle and as we're about to release RC3, it's far too late to fix it during this cycle :)

We can of course fix it in 6.3, but we can also consider this for 6.2.1. For the moment, I'm moving it to 6.2.1 for visibility.

#2 @abitofmind
15 months ago

Thanks for the first reaction. 6.2.1 then, certainly good!

This ticket was mentioned in PR #3981 on WordPress/wordpress-develop by @adamsilverstein.


15 months ago
#3

  • Keywords has-patch added

Match the naming behavior for uploaded media in the REST API to the way media is named when uploading in the media library.

Fixes https://github.com/WordPress/gutenberg/issues/34149

Trac ticket: https://core.trac.wordpress.org/ticket/57957

#4 @hellofromTonya
15 months ago

  • Description modified (diff)

Adding a link to the original bug report from Gutenberg.

@hellofromTonya commented on PR #3981:


15 months ago
#5

Description updated. This PR is now linked to the Trac ticket ✅

@adamsilverstein are there existing unit tests to validate this fix? If no, would you mind adding tests?

#6 @adamsilverstein
15 months ago

  • Keywords needs-unit-tests added

@adamsilverstein are there existing unit tests to validate this fix? If no, would you mind adding tests?

Good suggestion @hellofromTonya, will do.

I'd also like to get some more REST API component maintainer feedback, maybe @TimothyBlynJacobs or @spacedmonkey can review?

@adamsilverstein commented on PR #3981:


15 months ago
#7

@adamsilverstein are there existing unit tests to validate this fix? If no, would you mind adding tests?

I'll take a look, thanks for the suggestion.

porg commented on PR #3981:


14 months ago
#8

Any idea when this will be ready?
And if it is ready, in which WP version will it be included?

Asking to estimate for myself whether my upcoming mass picture placement session can count on this feature already or whether I must use a workaround. Thanks.

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


14 months ago

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


14 months ago

#11 @audrasjb
14 months ago

  • Milestone changed from 6.2.1 to 6.2.2

Since WordPress 6.2.1 Release candidate 1 is planned today, let's move this ticket to 6.2.2 to give it more time to add unit test cases and proper testing.

#12 @abitofmind
14 months ago

And regarding unit tests: What's the holdup? Any progress?

#13 @audrasjb
13 months ago

@abitofmind for the moment, this patch still needs unit tests.

Anyone is welcome to refresh the patch to add unit test cases, so this ticket could be considered for the next minor or major release.

#14 @abitofmind
13 months ago

My question is: What's the challenge with the unit tests? And what timeline to expect roughly.

#15 @audrasjb
13 months ago

No specific challenge, we are just waiting for someone wanting to handle them. No timeline for now as we're still waiting for these tests :)

#16 @desrosj
13 months ago

  • Milestone changed from 6.2.2 to 6.2.3

Moving this to 6.2.3.

#17 @abitofmind
13 months ago

#44789 was marked as a duplicate.

#18 @abitofmind
13 months ago

I now found #33789 which is already known for 5 years, and still unfixed.

#19 @adamsilverstein
12 months ago

57957.diff includes a new unit test to validate the fix, also on the PR

@adamsilverstein commented on PR #3981:


12 months ago
#20

are there existing unit tests to validate this fix? If no, would you mind adding tests?

@hellofromtonya - tests added

#21 @adamsilverstein
12 months ago

  • Keywords has-unit-tests needs-testing reporter-feedback added; needs-unit-tests removed

@abitofmind - can you test if the latest patch fixes the issue for you?

#22 @adamsilverstein
12 months ago

  • Focuses rest-api added
  • Owner set to adamsilverstein
  • Status changed from new to assigned

#23 @adamsilverstein
12 months ago

While the unit tests passed, this didn't actually work in Gutenberg as written: no "Content-Disposition" header is being passed.

Instead we can use the $files data which contains the original filename of the uploaded file, from which we can extract the correct media title. I have updated the PR to use this approach.

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


12 months ago

#26 @SergeyBiryukov
10 months ago

  • Milestone changed from 6.2.3 to 6.4

Moving to 6.4 for now, as there are no plans for 6.2.3 at this time, and this does not appear to be a regression in 6.3.

#27 @abitofmind
10 months ago

What needs to happen next that we get progress?

  • Must this whole issue be handled "elsewhere in core"?
  • Or is this just a hint/reference to apply the same practices for this very issue?

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


10 months ago

#29 @adamsilverstein
10 months ago

I see there was some additional feedback on the PR from @spacedmonkey that still needs addressing. I have been focused elsewhere so if anyone else want to pick this one up, please do so!

#30 @oglekler
9 months ago

  • Keywords changes-requested added; needs-testing removed

Because the patch needs a bit of work and some additional test coverage, I am adjusting keywords.

#31 @oglekler
9 months ago

@spacedmonkey I know that you are very popular person to ping, but if you are already know something about this issue, it will be much easier for you to tell what needs to be done to fix it.

Thank you! 🙏🙂

#32 @spacedmonkey
9 months ago

@oglekler I am still awaiting response / actioned feedback from @adamsilverstein. Sadly I don't think I have time to work on this change myself. But if Adam can take the time to look at into, I will provide code review.

#33 @nicolefurlan
8 months ago

@adamsilverstein with 6.4 RC1 rapidly approaching, I'm wondering if you think this fix will make it in this release cycle. Would it would be better to move it to a Future Release?

#34 @adamsilverstein
8 months ago

Apologies I have been focused elsewhere (mostly post meta revisioning) and haven't had a chance to get back to this yet. Feel free to punt if needed.

#35 @hellofromTonya
8 months ago

  • Milestone changed from 6.4 to 6.5

With the last scheduled 6.4 beta released today (beta 3) and committers working on this not being available, moving this ticket to 6.5. That said, if a fix is completed before RC1 and there's consensus it needs or should go into 6.4, please move it back into the milestone.

#36 @abitofmind
5 months ago

Could someone please write that missing unit test(s) so that this can finally get shipped?

#37 @swissspidy
4 months ago

  • Milestone changed from 6.5 to 6.6

#38 @adamsilverstein
2 weeks ago

  • Severity changed from major to normal

57957.2.diff is refreshed against trunk, includes tests, and addresses all feedback on the PR (I hope). @spacedmonkey please review again if you haven't had a chance to.

@adamsilverstein commented on PR #3981:


2 weeks ago
#39

@spacedmonkey the code you linked to leverages $_FILES[ $file_id ]['name']; - would we have access to that here?

The use of sanitize_text_field might not be appropriate in the REST handler where sanitization would be expected to happen elsewhere?

#40 @spacedmonkey
8 days ago

  • Keywords commit added; changes-requested removed

Marking as commit. Take it away @adamsilverstein

#41 @rajinsharwar
7 days ago

Test Report

Description

This report validates that the PR 3981 works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/3981

Environment

  • WordPress: 6.6-alpha
  • PHP: 8.1.23
  • Browser: Chrome
  • OS: Windows
  • Site - Multisite environment

Actual Results

  1. ✅ Text changes work fine with the patch.

Screenshots

Before:

https://img001.prntscr.com/file/img001/a9BXyZMrQGuHqox6m9j7Lw.png

After:

https://img001.prntscr.com/file/img001/4ETbj_igQU-PTqvjNM67cg.png

Note: See TracTickets for help on using tickets.