Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug where server side upload errors disappear automatically #7386

Merged
merged 1 commit into from
Jun 21, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Jun 19, 2018

Fixes: #7372
Partially reverts: #6957

Unfortunately, the approach used in #6957 to add upload error messages to all blocks without changing them does not seems possible, it created a regression as described in #6957, props to @danielbachhuber for catching this bug.

What happens is that when we upload we create a temporary blob URL for the file so that the placeholder disappears and the uploaded file appears immediately. In this case, the placeholder is unmounted. If we get an error message from the server e.g.: not something we can validate on the client, we set an error message but the placeholder is already not visible. Then a new placeholder is rendered, but its state is empty and the error message we set is not used anywhere.
This makes impossible to handle the notices in the media placeholder and forces them to be managed at the block level.

This PR does the following changes:
Reverts the changes by #6957 in the gallery, image, and media-placeholder.

Adds upload error notices to video, audio and cover image.

Solves an existing bug in the video, audio, and cover image blocks where if the upload failed, we continued with a blob URL set.

How has this been tested?

I added the following blocks with empty placeholders: video, audio, image, gallery, and cover-image.

For each of them, I uploaded a file with success. In audio block, it was not possible to upload mp3 files, but wav files were ok. I got the error message implemented in #6968. This happens because of a google chrome bug where audio/mp3 mimetype is used for audio instead of the correct one audio/mpeg. More info in :https://bugs.chromium.org/p/chromium/issues/detail?id=227004. I added a work around in #7398.

I added the following test code that forces server side upload errors:

$force_upload_failure = function( $file ) {
	$file['error'] = 'This file upload has been forced to fail.';
	return $file;
};
add_filter( 'wp_handle_sideload_prefilter', $force_upload_failure );
add_filter( 'wp_handle_upload_prefilter', $force_upload_failure );

I tried to upload to all the blocks and verified an error correctly appears on the block placeholder.

I change the max upload error size of my test site using htaccess:

php_value upload_max_filesize 3M
php_value post_max_size 3M

Tried to upload files bigger than 3M to all the blocks and verified a max file size error message appeared correctly.

@jorgefilipecosta jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Jun 19, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Jun 19, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the fix/bug-on-server-side-upload-errors branch from 5950495 to 8df66cf Compare June 20, 2018 08:13
@jorgefilipecosta jorgefilipecosta added this to the 3.1 milestone Jun 20, 2018
@jorgefilipecosta jorgefilipecosta changed the title WIP Fix bug where server side upload errors may disappear automatically Fix bug where server side upload errors may disappear automatically Jun 20, 2018
@jorgefilipecosta jorgefilipecosta changed the title Fix bug where server side upload errors may disappear automatically Fix bug where server side upload errors disappear automatically Jun 20, 2018
@jorgefilipecosta jorgefilipecosta added [Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended and removed [Status] In Progress Tracking issues with work in progress labels Jun 20, 2018
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Works in my testing.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/bug-on-server-side-upload-errors branch from 8df66cf to 934aaa7 Compare June 21, 2018 11:32
@jorgefilipecosta jorgefilipecosta merged commit 5ea93cd into master Jun 21, 2018
@jorgefilipecosta jorgefilipecosta deleted the fix/bug-on-server-side-upload-errors branch June 21, 2018 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants