Introduce `editorMediaUpload`, which includes `post` context for uploads #6231

danielbachhuber · 2018-04-17T23:48:16Z

Description

Adds a editorMediaUpload wrapper, which injects post context for media uploads.

Fixes #1586
Previously #6155

How has this been tested?

I tested by uploading a new image into Gutenberg:

Once the image was uploaded, it was correctly assigned to its parent post:

I also tested with an audio file.

noisysocks

👍 ran through your testing steps and it totally works!

noisysocks · 2018-04-18T01:49:08Z

blocks/editor-media-upload/index.js

+import { select } from '@wordpress/data';
+import { mediaUpload } from '@wordpress/utils';
+
+export default function editorMediaUpload( filesList, onFileChange, allowedType ) {


Let's add a doc comment to this function since it's exported.

noisysocks · 2018-04-18T01:57:00Z

utils/mediaupload.js

 	// Create upload payload
 	const data = new window.FormData();
 	data.append( 'file', file, file.name || file.type.replace( '/', '.' ) );
+	data.append( 'post', postId );


Could you please add a test case for this to utils/test/mediaupload.js?

Edit:: ...oh, just realised that file doesn't really contain anything juicy. It would be nice if we mocked wp.apiRequest and asserted that the POST request we build is what we expected. Up to you, though.

It would be nice if we mocked wp.apiRequest and asserted that the POST request we build is what we expected. Up to you, though.

I'm going to hold off for now. I think e2e tests will be more durable in the long term, but I'm still ramping up on where things currently stand.

postId is optional, should it be omitted in the POST data when not set?

gziolo · 2018-04-18T07:14:46Z

utils/mediaupload.js

@@ -12,8 +12,9 @@ import { compact, get, startsWith } from 'lodash';
 * @param {Array}    filesList    List of files.
 * @param {Function} onFileChange Function to be called each time a file or a temporary representation of the file is available.
 * @param {string}   allowedType  The type of media that can be uploaded.
+ * @param {number}   postId       Optional post id to associate with.


Should be {?number} to inform about possible null default value.

gziolo · 2018-04-18T07:21:00Z

utils/mediaupload.js

 */
-export function mediaUpload( filesList, onFileChange, allowedType ) {
+export function mediaUpload( filesList, onFileChange, allowedType, postId = null ) {


I started to think if this shouldn't be more general than postId. wp.utils was designed to contain general purpose utilities. An alternative approach would be to provide context param or additionalData param instead.

Then implementation inside createMediaFromFile could look like:

import { forEach } from 'lodash'; forEach( additional.data, ( ( value, key ) => data.append( key, value ) );

and you could refactor the code in the wp.blocks module as follows:

mediaUpload( filesList, onFileChange, allowedType, { post: select( 'core/editor' ).getCurrentPostId(), } );

Handled in 21f40c7

youknowriad · 2018-04-18T07:54:16Z

blocks/editor-media-upload/index.js

+ * @param {string}   allowedType  The type of media that can be uploaded.
+ */
+export default function editorMediaUpload( filesList, onFileChange, allowedType ) {
+	mediaUpload( filesList, onFileChange, allowedType, select( 'core/editor' ).getCurrentPostId() );


select( 'core/editor' ).getCurrentPostId() might be undefined here because the blocks module doesn't depend on the editor module. We thought for a long time that the blocks module can be used independently form the editor module but there are several places where this assumption has been proven wrong (autocompleter, shared blocks ...).

I think we're going to refactor these two modules to consolidate all components in editor and leave blocks for registration purpose only. The first step would be to extract the library of blocks to a separate module.

Not certain how to move forward for now though. Maybe we can check for the existence of the selector before calling it? and add a clarifying comment to move this to editor once the refactoring happens.

cc @gziolo @aduth @mtias another proof that this separation is not great.

Handled in 21f40c7 and 25bf867

@youknowriad, I don't need more proofs. I'm already sold on the idea of having API based blocks, core-blocks and having all UI components moved from block to editor :)
I would start with moving block/library to core-blocks module and make it dependent on both blocks and editor. It might require exposing some files from blocks but otherwise should be an easy task.

gziolo · 2018-04-18T13:30:07Z

Code looks good with the assumption that we will move this newly introduced function to editor module in the near future 👍 I haven't tested yet.

danielbachhuber · 2018-04-20T13:15:40Z

@WordPress/gutenberg-core Any other changes needed on this, or is it good to merge?

gziolo

It works as advertised. I wasn't aware I need to verify that in Media tab outside of Gutenberg. It took me a while to find out :)

I handled these initial requests.

danielbachhuber · 2018-04-20T15:50:44Z

I wasn't aware I need to verify that in Media tab outside of Gutenberg. It took me a while to find out :)

@gziolo Sorry about that!

youknowriad · 2018-04-20T15:52:07Z

Can I ask something related but not that important :) Why does WP keep a relationship between the post and the media? Doesn't seem so useful at first sight but I'm pretty sure I'm missing something :)

danielbachhuber · 2018-04-20T15:57:02Z

Why does WP keep a relationship between the post and the media? Doesn't seem so useful at first sight but I'm pretty sure I'm missing something :)

I'm unsure of its historical genesis but it has a variety of practical applications. For instance, if you use the [gallery] shortcode within a post, the default behavior is to display all images attached to the post. Similarly, an attachment has a post status of inherit, which means it inherits the visibility permissions of its parent post (when attached to the parent post).

joemcgill · 2018-04-20T16:49:16Z

Interestingly, this comment from Boren seems to give the best historical context for why attachments originally existed:

Looking forward to the day when users don't ever see the terms attach or detach. This whole thing is a hack around the lack of API for getting images from content.

…ads (WordPress#6231) * Introduce `editorMediaUpload`, which includes `post` context for uploads * Update media blocks to use `editorMediaUpload` and persist `post` * `mediaUpload()` doesn't return anything * Add JSDoc for function * Use an abstraction that permits passing additional data in request * Mention why this conditional is here

mtias · 2018-05-03T11:32:20Z

This whole thing is a hack around the lack of API for getting images from content.

Good thing blocks provide a path for this :)

danielbachhuber added 2 commits April 17, 2018 16:43

Introduce editorMediaUpload, which includes post context for uploads

441f0e3

Update media blocks to use editorMediaUpload and persist post

cff831f

danielbachhuber requested a review from a team April 18, 2018 00:02

danielbachhuber added this to the 2.7 milestone Apr 18, 2018

noisysocks previously requested changes Apr 18, 2018

View reviewed changes

danielbachhuber added 2 commits April 17, 2018 19:53

mediaUpload() doesn't return anything

8c30d8d

Add JSDoc for function

a004324

gziolo reviewed Apr 18, 2018

View reviewed changes

youknowriad reviewed Apr 18, 2018

View reviewed changes

youknowriad modified the milestones: 2.7, 2.8 Apr 18, 2018

danielbachhuber added 2 commits April 18, 2018 04:42

Use an abstraction that permits passing additional data in request

21f40c7

Mention why this conditional is here

25bf867

Merge branch 'master' into 6155-editor-media-upload

11fcd17

gziolo approved these changes Apr 20, 2018

View reviewed changes

danielbachhuber merged commit f53590a into master Apr 20, 2018

danielbachhuber deleted the 6155-editor-media-upload branch April 20, 2018 15:50

karmatosed added the [Feature] Media Anything that impacts the experience of managing media label May 3, 2018

This was referenced Dec 27, 2023

Site editor: fix image upload bug #57040

Merged

Media: send numerical post id when uploading image #57388

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `editorMediaUpload`, which includes `post` context for uploads #6231

Introduce `editorMediaUpload`, which includes `post` context for uploads #6231

danielbachhuber commented Apr 17, 2018 •

edited

Loading

noisysocks left a comment

noisysocks Apr 18, 2018

danielbachhuber Apr 18, 2018

noisysocks Apr 18, 2018 •

edited

Loading

danielbachhuber Apr 18, 2018

gziolo Apr 18, 2018

gziolo Apr 18, 2018

gziolo Apr 18, 2018 •

edited

Loading

danielbachhuber Apr 18, 2018

youknowriad Apr 18, 2018 •

edited

Loading

danielbachhuber Apr 18, 2018

gziolo Apr 18, 2018

gziolo commented Apr 18, 2018

danielbachhuber commented Apr 20, 2018

gziolo left a comment

danielbachhuber commented Apr 20, 2018

youknowriad commented Apr 20, 2018

danielbachhuber commented Apr 20, 2018

joemcgill commented Apr 20, 2018 •

edited

Loading

mtias commented May 3, 2018

Introduce editorMediaUpload, which includes post context for uploads #6231

Introduce editorMediaUpload, which includes post context for uploads #6231

Conversation

danielbachhuber commented Apr 17, 2018 • edited Loading

Description

How has this been tested?

noisysocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Apr 18, 2018

danielbachhuber commented Apr 20, 2018

gziolo left a comment

Choose a reason for hiding this comment

danielbachhuber commented Apr 20, 2018

youknowriad commented Apr 20, 2018

danielbachhuber commented Apr 20, 2018

joemcgill commented Apr 20, 2018 • edited Loading

mtias commented May 3, 2018

Introduce `editorMediaUpload`, which includes `post` context for uploads #6231

Introduce `editorMediaUpload`, which includes `post` context for uploads #6231

danielbachhuber commented Apr 17, 2018 •

edited

Loading

noisysocks Apr 18, 2018 •

edited

Loading

gziolo Apr 18, 2018 •

edited

Loading

youknowriad Apr 18, 2018 •

edited

Loading

joemcgill commented Apr 20, 2018 •

edited

Loading