Upload image issue after deleting image from edit image page

[rnitinb]

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

[rnitinb]

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.

[Mista-Flo]

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]

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.

[antpb]

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

[desrosj]

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?

[hellofromTonya]

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

[joedolson]

Following some exploration, I find that the display messages in grid view are being sent to the browser, they're just always hidden. They're sent to .media-sidebar .media-uploader-status.

This seems to mostly be a CSS thing. If you remove display:none from .upload-php .mode-grid .hide-sidebar .media-sidebar, then the message region appears and gives some very basic info, but it's not toggling all of the other style features.

Needs more research, but seems solvable.

[antpb]

There is some complexity here that require this to wait one release. Moving to 6.1 so we have enough time to test and fix.

[antpb]

The above comment is still valid. Moving to 6.2 to keep this on the Media team's radar.

[zebaafiashama]

This report validates that the indicated patch addresses the issue.
Patch tested: https://core.trac.wordpress.org/attachment/ticket/49178/49178.1.patch
Environment
OS: Windows 22621.1105
Web Server: nginx/1.16.0
PHP: 8.1.9
WordPress: 6.1.1
Browser: Chrome 109.0.5414.74
Theme: Twenty Twenty-Two
Active plugins: N/A

Actual Results
-Before adding the code if from media upload a image and edit with details and then delete it but again when try to upload the same page from add new image ,it was not showing
-After added the code if the image is deleted and try to upload it again it will show
Before adding code
​https://d.pr/i/1GaUX9
After Adding code
​https://d.pr/v/LARp6A

[joedolson]

Refreshed patch

[joedolson]

Patch refreshed and tested; confirmed testing results from @zebaafiashama

Looking at the changes, they're ultimately pretty simple: a little re-ordering and unsetting the GET variables. The fact that this allows confirmation messages to display in the grid view is, in my opinion, a significant improvement.

[joedolson]

One possibility for some code simplification here:

+	if ( 1 === $detached ) {
+		$message = __( 'Media file detached.' );
+	} else {
+		/* translators: %s: Number of media files. */
+		$message = _n( '%s media file detached.', '%s media files detached.', $detached );
+	}

Each check includes an if/else structure gives a different message depending on whether the result is singular or plural. The value of $detached (in this example) is an absint, so it's always going to be a non-negative integer; so the singular return value of _n() here would never be used. I can't see any reason these couldn't be collapsed to remove the if/else.

[joedolson]

Alternately, the plural string could be fixed, rather than using _n. I also note that the function number_format_i18n() is not needed here, as this value will never be a float.

[joedolson]

Updated patch

[joedolson]

Updated patch removes number_format_i18n, as the value has been passed through absint and will always be an integer & fixes a non-strict comparison. Does not change the translatable strings.

[joedolson]

The argument in favor of keeping the existing strings, as I see it, is that

1) The text "1 media item detached" adds a little more mental overhead to understand than "media item detached"

and

2) A translatable string that only supports plurals could confuse translators, who would expect to encounter both plural and singular strings.

To that effect, I think that not changing those strings might be the least friction choice. Open to 2nd opinions, though.

[antpb]

@joedolson I think that change is correct. If handled upstream it should be covered in this case.

Test patch to fix upload image issue after deleting image from edit image page

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

[joedolson]

In 55178:

Media: Fix upload not visible if added after deleting media.

Unset GET parameters in the media library so they don't suppress error notifications and messages. Fix an issue where uploading an image after being redirected from deleting media did not show the new upload in the gallery.

Props rnitinb, Mista-Flo, desrosj, zebaafiashama, joedolson, antpb.
Fixes #49178.

[SergeyBiryukov]

Replying to joedolson:

One possibility for some code simplification here:

+	if ( 1 === $detached ) {
+		$message = __( 'Media file detached.' );
+	} else {
+		/* translators: %s: Number of media files. */
+		$message = _n( '%s media file detached.', '%s media files detached.', $detached );
+	}

Each check includes an if/else structure gives a different message depending on whether the result is singular or plural. The value of $detached (in this example) is an absint, so it's always going to be a non-negative integer; so the singular return value of _n() here would never be used. I can't see any reason these couldn't be collapsed to remove the if/else.

Just to follow up here, the singular return value of _n() would actually be used in languages that have more than one plural form, see a note in the ​plugin i18n handbook:

Note that some languages use the singular form for other numbers (e.g. 21, 31 and so on, much like ’21st’, ’31st’ in English). If you would like to special case the singular, check for it specifically:

if ( 1 === $count ) {
    printf( esc_html__( 'Last thing!', 'my-text-domain' ), $count );
} else {
    printf( esc_html( _n( '%d thing.', '%d things.', $count, 'my-text-domain' ) ), $count );
}

For more context:

• Changesets:
  • [31941]: Decouple strings where the singular and plural form are not just the same string with different numbers, but essentially two different strings.
  • [31951]: After [31941], use the decoupled strings from WP_Customize_Manager::register_controls() on the Menus screen.
  • [32029]: After [31941], use the decoupled strings from wp-admin/network/themes.php in wp-admin/network/site-themes.php as well.
  • [34521]: Plugins: Don't use _n() for singular/plural strings which have no placeholder for a number.
  • [35611]: About: Don't use _n_noop() for singular/plural strings which provide no placeholder for a number.
• Comments:
  • comment:5:ticket:28502
  • comment:23:ticket:28502
  • comment:37:ticket:28502
• Tickets: #28502, #33239, #34127, #34307, #34308.

Replying to joedolson:

I also note that the function number_format_i18n() is not needed here, as this value will never be a float.

I believe it is best practice to always include number_format_i18n() when using sprintf() with _n(), as it not only deals with float values, but also formats the thousands separator according to the locale. It might not be common to perform an action on more than a thousand of media items at once, but it's technically possible, and including number_format_i18n() would be more consistent with the rest of core.

Removing the number_format_i18n() calls in [55178] does not appear to be related to that particular fix, so I have restored them in [55631] as part of some other cleanup :)