Site editor: fix image upload bug #57040
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glendaviesnz
added
[Type] Bug
|
I did initially try to find a way to inject a different mediaUpload method into the site editor settings as used to happen, but I think this simple fix achieves the same thing. |
|
I think we should add an additional e2e test for image load in site editor. This can be done in a separate PR though. |
|
Size Change: +21 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
aaronrobertshaw
approved these changes
Dec 14, 2023
There was a problem hiding this comment.
LGTM 👍
✅ Could replicate image upload issue in the site editor
✅ Applying the changes in this PR fix the issue
✅ Post editor image uploads still work
As noted, we need an e2e to cover image uploading in the site editor. No objections here though for that to be done in a follow-up.
|
Flaky tests detected in c667ed4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7204473157
|
talldan
approved these changes
Dec 14, 2023
There was a problem hiding this comment.
I think this is good as an interim fix, but there might need to be some thought about whether the way mediaUpload works is right in the context of the site editor.
Seems like we might not be able to rely on getCurrentPostId.
Would appreciate thoughts from @youknowriad, as it seems related to some changes he's been making.
| uploadMedia( { | ||
| allowedTypes, | ||
| filesList, | ||
| onFileChange, | ||
| additionalData: { | ||
| post: getCurrentPostId(), | ||
| ...( ! isNaN( currentPostId ) ? { post: currentPostId } : {} ), |
There was a problem hiding this comment.
Nitpick, but I'd find it more readable if it were something along the lines of:
const postData = isNaN( currentPostId ) ? {} : { post: currentPostId };
// ...
additionalData: {
...postData,
...additionalData
};
There was a problem hiding this comment.
In the case of templates, the isNan test will return false. Is that a problem?
! isNaN('twentytwentyfour//home') === false // true
| @@ -35,13 +35,13 @@ export default function mediaUpload( { | |||
| const wpAllowedMimeTypes = getEditorSettings().allowedMimeTypes; | |||
| maxUploadFileSize = | |||
| maxUploadFileSize || getEditorSettings().maxUploadFileSize; | |||
|
|
|||
| const currentPostId = getCurrentPostId(); | |||
There was a problem hiding this comment.
I wonder what the post id is used for? Attachments?
It seems like the way this function works might lead to some issues. In the site editor an image could be uploaded in the context of a post if you were editing post content, but I'm not sure getCurrentPostId is guaranteed to return the right thing ... maybe there are some contexts where it returns the template id instead of the content post id (as per this bug).
I wonder if the post id should really be passed into mediaUpload in the majority of cases, and maybe determined using useEntityProp within the block.
There was a problem hiding this comment.
When I edited a page in the context of the site editor with this patch the post id was set correctly in this method. But previously with the site editor specific method no post id was being set for the media upload even when editing in the context of the post/page, so it seems like moving to this shared method might have resolved an existing potential issue as well as causing the bug. As you note though, there may be some contexts in the site editor where it isn't returned correctly so could be worth some further investigation.
I don't know what the impact might be of all the images previously uploaded in pages in the site editor not having the post id set might be - so it could be worth looking further into why it is set. I will look at that tomorrow if @youknowriad doesn't know.
There was a problem hiding this comment.
maybe there are some contexts where it returns the template id instead of the content post id (as per this bug).
No, it always returns the postId no matter the context.
There was a problem hiding this comment.
That said, when you open a template directly in the site editor, it contains the template id, when you open the navigation block, it contains its id, when you open a pattern or template part, same...
There was a problem hiding this comment.
I guess I was thinking of whether it's possible to open a template (where it's set to the template slug) and the edit an image inside the Content block of a nested post, but that doesn't seem possible in my testing.
There was a problem hiding this comment.
so it could be worth looking further into why it is se
Original PR here:
Since that PR, editorMediaUpload has been renamed to mediaUpload and refactored a bit.
Here's the reason it seems:
Once the image was uploaded, it was correctly assigned to its parent post
There was a problem hiding this comment.
Having said that, I'm not entirely sure sending the post id is required at all 🤷🏻
But it's been there so long it's expected, at least by Jetpack. See: Automattic/wp-calypso#85373 (comment)
Here's a PR that forces the numerical ID:
There was a problem hiding this comment.
Yes, here we go. Right in front my eyes. The post arg is an integer according to the WP_REST_Attachments_Controller
fullofcaffeine
pushed a commit
that referenced
this pull request
Dec 26, 2023
…pending to upload media data (#57040)
artemiomorales
pushed a commit
that referenced
this pull request
Jan 4, 2024
…pending to upload media data (#57040)
fullofcaffeine
pushed a commit
that referenced
this pull request
Jan 6, 2024
…pending to upload media data (#57040)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes a bug with images not uploading in the site editor.
Fixes: #57037
Why?
In #55970 the settings were shared between site editor and post editor, but the site editor image upload did not previously include a post id as the post id in site editor is a template string slug. This string slug is now being passed as a post id to the image upload endpoint which causes a bad request error.
How?
Checks for a numeric post id before appending to upload media data.
Testing Instructions
Uploadoption and make sure it succeedsScreenshots or screencast
Before:
image-before.mov
After:
image-after.mov