WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 7 weeks ago

#39968 reopened defect (bug)

Media Library: deleting all items on the last page loses the pagination/navigation buttons and shows message

Reported by: donsony Owned by: antpb
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7.2
Component: Media Keywords: has-patch needs-testing
Focuses: ui Cc:

Description

While deleting all items on the last page of Media Library, the page loses the pagination/navigation buttons and shows the message "No Media Files found" with no way to access the earlier pages other than by going back to the "Media" link

Attachments (4)

Screen Shot 2018-12-20 at 3.09.37 PM.png (122.6 KB) - added by antpb 2 years ago.
illustration of the behavior
39968.1.patch (621 bytes) - added by Mista-Flo 8 months ago.
r49567-bug.png (56.6 KB) - added by trepmal 5 months ago.
inaccurate item total with r49567
39968.2.patch (1.0 KB) - added by Mista-Flo 3 months ago.
Second patch

Download all attachments as: .zip

Change History (31)

#1 @karmatosed
4 years ago

  • Summary changed from Media Library to Media Library: deleting all items on the last page loses the pagination/navigation buttons and shows message

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


2 years ago

@antpb
2 years ago

illustration of the behavior

#3 @antpb
2 years ago

I've added an illustration of the behavior above, we have two options here that @desrosj outlined in the recent #core-media meeting:

"I would either return the user to page 1, or, send them to the _new_ last page."

#4 @desrosj
2 years ago

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

I looked at how other admin areas handle this. This is what I found:

  • Posts: reloads the page with paged - 1 (new last page).
  • Categories: reloads the page with paged - 1 (new last page).
  • Users: This flow is a bit different because there is a confirmation page in between. But, the user is always returned to the first page of users after deleting users.

My vote is to keep things consistent with the other post and term areas in core and redirect the user to the new last page (paged-1).

#5 @Mista-Flo
8 months ago

  • Keywords has-patch added; needs-patch removed

Hi there, it took me ages to understand what was wrong here.

For example, if you have just two pages of attachments and you are on page 2, just change the paged argument in the URL to 37 and it still loads the UI with no attachments at all.

After some digging, I have found out that $wp_query->found_posts and $wp_query->max_num_pages are equal to 0.

So even if it's on page 450 that does not exist, the posts list table have a count of the number of total posts which helps to identify the maximum number of pages in the pagination and redirect to the last existing page if the requested page is too high, check set_pagination_args function in wp-admin/includes/class-wp-list-table.php for more information.

So finally I just used the wp_count_attachments() function to mimic the behaviour of the post list table and it worked.

@Mista-Flo
8 months ago

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


7 months ago

#7 @justinahinon
7 months ago

39968.1.patch is fine and correctly solve the issue on my Trunk version.
When I delete all the items in the last page, it returns to the new last page.

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


7 months ago

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


7 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.


5 months ago

#12 @antpb
5 months ago

  • Keywords commit added
  • Milestone changed from Future Release to 5.6

This works well in my testing and is related to another patch in 5.6. Because of this, we've moved this ticket into the milestone ahead of Beta 4.

#13 @antpb
5 months ago

  • Owner set to antpb
  • Resolution set to fixed
  • Status changed from new to closed

In 49567:

Media: Improve count in Media Library pagination.

Deleting all visible items on the last page of the media library previously left a blank page with no media items available. Using wp_count_attachements instead of found_posts solves the problem.

Props donsony, karmatosed, desrosj, mista-flo, justinahinon.

Fixes #39968.

#14 @iCaleb
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi all,

This changeset means that the total count and pagination in the media library will no longer take additional query filtering into account.

Example issue we just ran into with this patch applied:

add_action( 'pre_get_posts', function( $query ) {
        $display_all = filter_input( INPUT_GET, 'example-filter', FILTER_SANITIZE_STRING );

        if ( 'all' !== $display_all ) {
                $query->set( 'meta_key', 'some_meta_key' );
        }
} );

Before this patch, the pagination/total count would be correct with and without the filtering. But now with the patch applied, the pagination displays far more pages than needed with the latter half all being blank.

I can open a new ticket if needed for this bug, but seemed more fitting to go here. Need to take another look at how to resolve this bug without removing the ability to filter the $wp_query.

@trepmal
5 months ago

inaccurate item total with r49567

#15 @trepmal
5 months ago

Adding onto @iCaleb's comment, a simpler demonstration of the issue can be found with the media type and date filters in list view. Note that the pagination area shows 38 items and 2 pages, but there is only result that matches the date filter.

https://core.trac.wordpress.org/raw-attachment/ticket/39968/r49567-bug.png

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


5 months ago

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


5 months ago

#18 follow-up: @peterwilsoncc
5 months ago

I've been able to reproduce this with @trepmal's steps above

  • upload media over the course of two or more months (you might need to mess with server time to do this, or run a production server on RC1)
  • view media library list view
  • in pagination, note the number of items and max pages displayed
  • limit images to a single month
  • observe neither of the number have changed in the pagination section

I suggest reverting [49567].

Filtering either via the admin or using WP_Query seems less of an edge case than deleting all the images on the final page.

#19 in reply to: ↑ 18 @SergeyBiryukov
5 months ago

Replying to peterwilsoncc:

I suggest reverting [49567].

I agree, that appears to be the safest option for RC2.

#20 @SergeyBiryukov
5 months ago

In 49720:

Media: Revert [49567].

This addresses a regression with the pagination section in Media Library no longer taking additional query filtering into account.

Props iCaleb, trepmal, peterwilsoncc.
See #39968.

#21 @SergeyBiryukov
5 months ago

In 49721:

Media: Revert [49567].

This addresses a regression with the pagination section in Media Library no longer taking additional query filtering into account.

Props iCaleb, trepmal, peterwilsoncc.
Reviewed by peterwilsoncc, SergeyBiryukov.
Merges [49720] to the 5.6 branch.
See #39968.

#22 @SergeyBiryukov
5 months ago

  • Keywords has-patch commit removed
  • Milestone changed from 5.6 to Future Release

Moving to a future release for further work.

@Mista-Flo
3 months ago

Second patch

#23 @Mista-Flo
3 months ago

Hey guys, I have uploaded another patch that fixes the issue with filters.

Unfortunately there are some backend logic somewhere else that prevent non existing pages to redirect to the last page when a filter search is made like with the date.

Steps to reproduce:

  1. Go to wp-admin/upload.php
  2. Enable list mode
  3. Filter by a given month
  4. Go to page 2
  5. Then directly change the paged parameter to a random page like 56
  6. It won't show any attachment, but it does not redirect you to the last existing page

So it seems to partially fix the original issue.

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


3 months ago

#25 @antpb
3 months ago

  • Keywords has-patch needs-testing added

@Mista-Flo that behavior is the current experience even without the fix. I personally don't think we have to solve for that edge case in this ticket. Might be good to document in a new ticket, but I think it's somewhat unrelated to the core issue here.

The code looks good from my high level view. Will test it locally here soon. :) thank you!

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


7 weeks ago

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


7 weeks ago

Note: See TracTickets for help on using tickets.