WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 8 weeks ago

#49178 new defect (bug)

Upload image issue after deleting image from edit image page

Reported by: rnitinb Owned by:
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.3.2
Component: Media Keywords: has-patch
Focuses: Cc:

Description

Hello,

I have found an issue in the image upload process.

Steps to recreate it:
1) First, open upload image page from wp-admin
2) Upload any image and then go to "Edit more details" page of uploaded image
3) Then, delete that image so it will redirect you to an upload page with ?deleted=1 parameter in a query string like http://localhost/elements/wp-admin/upload.php?deleted=1

OR

3)Just add ?deleted=1 after upload.php page of upload image page like http://localhost/elements/wp-admin/upload.php?deleted=1
4) Then, upload a new image for show issue.

The issue is that, after deleting an image and if you upload an image, an image can not be displayed in the image gallery. You must refresh the page to see that image.

The issue is in the video => https://www.youtube.com/watch?v=_S6z3BySB64

Attachments (1)

49178.1.patch (8.7 KB) - added by Mista-Flo 11 months ago.

Download all attachments as: .zip

Change History (16)

#1 @SergeyBiryukov
19 months ago

  • Component changed from General to Media

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


18 months ago

#3 @rnitinb
15 months ago

Hello,

I have submitted this issue 4 months ago but there is no update on this yet.

This issue still exists in WordPress 5.4.1, please take a look into this.

Thanks & regards.

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


14 months ago

#5 @swissspidy
14 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#6 @Mista-Flo
11 months ago

I'm looking at this issue.

First, before looking at the ticket issue, I have to say that the upload.php is a tough one. It handles the both list view and grid view in a very different way. The grid mode triggers a different markup at all, it loads a JS underscore template. One of the drawbacks with the current implementation IMO is that we don't display messages in grid view. In list view, if you do the same actions, you would have a notice just above the filters saying that the file has been permanently deleted. I don't see any reason why that's not the case either for grid view. The bad thing is that there is no easy way to just use the same code (about messages) for both context.

@Mista-Flo
11 months ago

#7 @Mista-Flo
11 months ago

  • Keywords has-patch added; needs-patch removed

Hi, I have submitted a patch that fixes the issue. I am not sure about the implementation though so let's give your feedback.

Here's the explanation of the patch :

The curent behaviour is: When a GET deleted param is present, in list mode, it directly removes it from the URL query args, and drop a message.

With my patch, it now displays messages even in grid mode, and also unset from the $_GET superglobal the deleted and other params like this that was avoiding new uploaded images to get displayed in the grid.

You can have a closer look to this code:

<?php
        $q = $_GET;
        // Let JS handle this.
        unset( $q['s'] );
        $vars   = wp_edit_attachments_query_vars( $q );
        $ignore = array( 'mode', 'post_type', 'post_status', 'posts_per_page' );
        foreach ( $vars as $key => $value ) {
                if ( ! $value || in_array( $key, $ignore, true ) ) {
                        unset( $vars[ $key ] );
                }
        }

It might be a better option to ignore deleted and other params set, but I am not sure about the consequences.

Last edited 11 months ago by Mista-Flo (previous) (diff)

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


11 months ago

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


10 months ago

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


6 months ago

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


6 months ago

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


5 months ago

#13 @antpb
5 months ago

  • Milestone changed from Future Release to 5.8

Mentioned in the recent Media Component meeting, this looks like a good ticket for 5.8. Adding to the milestone for visibility.

#14 @desrosj
2 months ago

So I did some testing with this today. Here's what I found.

While using list view for the media library, when an image is deleted a message is displayed correctly for all scenarios and the query variable is correctly removed from the URL. This happens because deleted is already returned in the list returned by wp_removable_query_args(), and wp_admin_canonical_url() changes the browser's location to a URL with those parameters removed. The user is always allowed to update a new image because it requires navigating to the wp-admin/media-new.php page.

When using the media gallery in grid view, no message is displayed when an image is deleted within any user flow (from the individual image edit screen, or the image overlay). deleted is only added when deleting from single attachment edit screen, and the parameter remains when redirected to the Media Library.

It appears to be intentional that the parameter is not removed, as the call to wp_admin_canonical_url() was removed in [34256] to prevent any query args required by the grid view from being removed. I removed this line to see if it fixed the issue. It did remove the deleted query arg, but I still was not shown the visual confirmation that an image was being uploaded, and no confirmation that an image was deleted was shown.

I'm still not sure why the deleted parameter would prevent the UI from functioning properly. Even though no visual elements or progress is displayed to the user, the image does upload successfully.

I'm also curious as to historical reasons behind the messages not being displayed in grid view. Did these ever display correctly? Or was it by design that no admin messages were displayed? The only one I can think of seeing recently is when an upload fails. @mikeschroder or @joemcgill do you have any knowledge there?

#15 @hellofromTonya
8 weeks ago

  • Milestone changed from 5.8 to 5.9

Today is 5.8 Beta 1. As discussions are ongoing, punting to 5.9.

Note: See TracTickets for help on using tickets.