Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50025 closed defect (bug) (fixed)

Media Library not showing new uploads when filtering by date

Reported by: teamdnk's profile teamdnk Owned by: adamsilverstein's profile adamsilverstein
Milestone: 5.7 Priority: normal
Severity: normal Version: 5.4
Component: Media Keywords: has-patch needs-testing
Focuses: ui, javascript Cc:

Description

The issue is produced while in the grid mode and while using the media modal since both use Ajax. The problem happens when attempting to upload a new image while using the filter(s). New uploads do not show up without refreshing the page. To reproduce do the following steps:

  1. Use grid mode in Media Library
  2. Filter by a date
  3. Upload a new image
  4. Toggle back to "All Dates"

You will notice that not only is there no feedback whether or not my image has been successfully uploaded, but the new image does not show even when toggling back to the "All Dates". The only way that I have found to show any new uploads is to actually refresh the page which defeats the purpose especially with the modal. The modal at least has some feedback with the uploading status bar but still has the same issue.

There should be some kind of feedback when uploading media while using the filters. At the very least it should show the new upload while on the current month and when toggling back to the "All" filter.

Attachments (2)

50025.diff (1.2 KB) - added by adamsilverstein 4 years ago.
50025.2.diff (2.0 KB) - added by adamsilverstein 4 years ago.

Download all attachments as: .zip

Change History (42)

#1 @afercia
4 years ago

  • Focuses administration removed

@teamdnk thanks for your report and welcome to Trac.

I can reproduce what you reported but only partially. It seems to me the actual behavior differs depending on the editor used and on the state of the media selection. Would you mind clarifying a few things?

  • I assume you're using WordPress 5.4, correct?
  • Are you using the Classic Editor or the Block Editor?
  • Do you have any activated plugin and are you using a default theme? If so, please confirm that the issue still exists with the default theme (Twenty Twenty) activated and all other plugins deactivated.

From what I can tell, based on my testing:

Media Library (grid mode)

  • when switching back to "All dates", the new media are visible for me

Media Modal with the Classic Editor

  • When uploading one or more images, they're automatically "selected" and the selection state is visible in the bottom bar:

http://cldup.com/TUziTp2K_w.png

http://cldup.com/21NCs75Eej.png

  • Keeping the selection: when I switch back to "All dates", the new media are visible for me.
  • Clearing the selection: when I switch back to "All dates", the new media are not visible. I think this has always been the case with the media modal. Not saying it shouldn't be fixed. It's just to clarify it's an issue that probably exists since the introduction of the media modal or, maybe, since the introduction of the "date" filter in WordPress 4.1.
  • In all cases, there's a lack of a clear feedback and this should be fixed in some way.

Media Modal with the Block Editor
It's a bit more complicated to test because different blocks use different media modal types. For example:

  • The Image block uses the "Select media" frame (no selection status bottom bar).
  • The Gallery block uses the "Create Gallery" frame (does have the selection status bar).
  • In both cases, the added images are visible for me.

So, at least for me, this appears to happen when the selection status is "cleared". Note that in the Media Library "grid", the added media are not automatically selected, which is probably the reason why the bug doesn't happen there.

#2 @teamdnk
4 years ago

@afercia

Thanks for the welcome and the response! For clarification:

  • WordPress 5.4
  • Block Editor
  • No plugins and using Twenty Twenty theme

I do not have the classic editor so I cannot speak to that but here is what I found:

Media Library (grid mode)
I noticed after flushing it out a little more that the issue happens when you toggle back to "All Dates" at least once before uploading new media. I am not sure but it seems to be caching the results with out re-querying new results after a new upload.

Media Modal with the Block Editor
I can replicate the same behavior on the Image block and the Gallery block.

#3 @afercia
4 years ago

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

@teamdnk thanks for further testing.

the issue happens when you toggle back to "All Dates" at least once before uploading new media.

it seems to be caching the results with out re-querying new results after a new upload.

Yup, the media views collections do implement a caching mechanism. They check whether a query has already been made and in that case they just re-display the cached collection, avoiding a new fetch. I'm not sure I fully understand how this mechanism works, yet. The fact the media views inner workings aren't described nor documented anywhere after more than 7 years from their introduction doesn't help.

Regardless, I was able to reproduce something similar to the steps you provided back to WordPress 4.9 and I guess it always happened on all previous versions:

  • go to the Media Library so that attachments are fetched for the first time
  • filter by a previous month
  • in the filtered month view, drag and drop a new attachment
  • switch back the filter to "All dates"
  • the new attachment is visible
  • but...
  • switch back to the previously selected month
  • drag and drop one more attachment
  • switch back the filter to "All dates"
  • this time, the new attachment is not visible

This translates to:

  • first fetch: the "all" view is not cached yet
  • switch to a month: the month view is not cached yet
  • add attachment, switch to "all" view
  • the "all" view is not cached yet: a new fetch occurs which includes the new attachment and the collection gets cached
  • switch to month: the month view is now cached
  • add attachment, switch to "all" view
  • the "all" view is cached and will display the previous attachments, ignoring the new one

To my understanding, there's a flaw in the media views collections design. When attachments are added, the cache should be flushed to trigger a new fetch.

This doesn't seem to happen when attachments are removed. Although the collection is cached (there's no new fetch), the deleted attachment is not displayed, as expected. Agani to my understanding, this works correctly because destroyed attachments are filtered out.

Moving to 5.5 consideration.

#4 @afercia
4 years ago

Additional detail: not to mention that, when the added attachment belongs to a new month, the "filter by date" select doesn't get updated with the new month. This select gets the month values from a query to the database that runs on page load. The values are printed out on the page under _wpMediaViewsL10n.settings thus the select isn't updated "on the fly".

#5 @teamdnk
4 years ago

@afercia

When attachments are added, the cache should be flushed to trigger a new fetch.

Thank you for breaking down explaining exactly what is going on here. I agree that the cache should be flushed after uploading a new attachment.

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


4 years ago

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


4 years ago

#8 @antpb
4 years ago

  • Milestone changed from 5.5 to 5.6

This ticket was discussed in a recent Media Component bug scrub for 5.5. I'm going to move this to 5.6 as we are in Beta cycle and this change may be a bit too big to add in Beta. I'm happy to be wrong on that and please feel free to move back into the milestone if it's possible to land in time. Thanks!

#9 @Mista-Flo
4 years ago

Note that I can reproduce the issue as well in the upload.php admin page in grid view. If I select the current month in the Date filter, even if I upload a new image, it's not shown on the grid.

I am not familiar with the operations made in JS here.

But what I can tell in the media library popup. When uploading a new file with a date filter, there is no prepare event that is triggered (in file /src/js/media/views/attachments.js) whereas it is if the date filter is not set.

Last edited 4 years ago by Mista-Flo (previous) (diff)

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


4 years ago

#11 @antpb
4 years ago

  • Milestone changed from 5.6 to Future Release
  • Summary changed from Media Library (Ajax) Filters Not Showing New Uploads to Media Library not showing new uploads when filtering by date

With 5.6 Beta 1 in the rear view mirror, we should probably mark this Future Release as there is still investigation to do here.
Also changing the title to better describe the issue as ajax doesn't seem to be the culprit.

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


4 years ago

#13 @antpb
4 years ago

  • Milestone changed from Future Release to 5.7

This was discussed in the recent Media meeting as being a blocker to process on #50105. Both of these are goals of 5.7 for Media so I'm going to be moving them into our next milestone. :)

#14 @adamsilverstein
4 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

I'll take a look at this one to see if I can get to the bottom of the issue.

This ticket was mentioned in PR #809 on WordPress/wordpress-develop by adamsilverstein.


4 years ago
#15

  • Keywords has-patch added; needs-patch removed

#16 @adamsilverstein
4 years ago

In 50025.diff:

  • Remove the query caching mechanism, which was buggy and did not support invalidation when the media collection changed.

The issue with the caching was that the cache was never invalidated. After digging into how to fix this for a while, I discovered invalidating this caching when a new image is uploaded is difficult. Because of the way the cache was wrapped up inside the query object, triggering an external invalidation would require significant re-architecting.

Instead, I have opted to remove the simplistic caching entirely; media will now be re-queried when you change filter settings, regardless of whether you have seen that filtered view before. I don't think the caching added much value for users.

@afercia Can you test this to verify it fixes the original issue you described (not the bit about date menu populating only at load time). It does in my testing.

@antpb does removing the caching here seem like a reasonable compromise?

This ticket was mentioned in Slack in #core-js by ocean90. View the logs.


4 years ago

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


4 years ago

#19 @joedolson
4 years ago

This patch appears to fix the issue @afercia was looking at. I agree that the caching doesn't appear to add significant value. I'll look at #50105 to see if this allows that to work as expected.

#20 @joedolson
4 years ago

  • Keywords commit added

@antpb @adamsilverstein I think that this change should be committed so we can get broader testing. While I don't think the caching was doing a lot, we need to verify this in a variety of environments to be sure. It doesn't fully solve this ticket, but it's an important stage.

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


4 years ago

#22 @antpb
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 50021:

Media: Remove caching from filter by date in media library.
Previously, newly uploaded media attachments were missing when filtering media items by date due to lack of cache invalidation.
Props adamsilverstein, teamdnk, afercia, Mista-Flo, joedolson.
Fixes #50025.

#23 @talldanwp
4 years ago

@antpb @adamsilverstein @joedolson Just spotted that the e2e tests in Gutenberg are failing, and I think it's from this change. It's a test related to adding media in the classic block.

I was able to reproduce manually:

  1. Open the post editor
  2. Add a classic block
  3. Use the 'Add Media' button and upload a new image (use the 'Add files' tab if you already have some media in the library).

Expected - once uploaded, the new image appears in the grid.
Actual - the uploaded image doesn't display in the grid.

Also tested in the classic editor and the same problem there, so definitely seems related to a recent core commit.

Last edited 4 years ago by talldanwp (previous) (diff)

#24 @antpb
4 years ago

In 50029:

Media: Reintroduce caching for Media Library query.

In [50021], caching was removed causing unintended classic block media flows to fail.

Reverts [50021].
See #50025.

#25 @antpb
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#26 @antpb
4 years ago

  • Keywords needs-patch added; has-patch commit removed

Thanks for the catch there @talldanwp. Really appreciate it! I've gone ahead and reverted that so we can take another look at a better solution that allows classic block to perform as it does today.

Pardon the interruption. :)

Marking needs-patch as we work to fix this.

#27 @adamsilverstein
4 years ago

Thanks for hopping on this @antpb - were you able to confirm the issue was fixed after reverting? I'll take a look at this to see if we can re-introduce change without breakage.

#28 @youknowriad
4 years ago

I can confirm that the Gutenberg e2e tests now pass :) It's funny that Gutenberg e2e tests catch Core bugs. We should probably invest more in e2e tests in Core.

This ticket was mentioned in PR #933 on WordPress/wordpress-develop by adamsilverstein.


4 years ago
#29

  • Keywords has-patch added; needs-patch removed

Trac ticket:

#30 @adamsilverstein
4 years ago

  • Keywords needs-testing added

50025.2.diff removes an additional spot the caching was used and fixes the bug noted above.

@talldanwp thanks for the steps to reproduce. In my testing the latest patch fixes the issue with the classic block (I didn't test in the classic editor). Can you please confirm it also works for you?

It's funny that Gutenberg e2e tests catch Core bugs. We should probably invest more in e2e tests in Core.

🥲 @youknowriad maybe as a start we can run the Gutenberg tests when PRs are opened against wordpress-develop?

#31 @youknowriad
4 years ago

@youknowriad maybe as a start we can run the Gutenberg tests when PRs are opened against wordpress-develop?

There's actually a script in core that does just that tests/gutenberg/run.js (the version const there might need to be updated)

We already use that script to run these tests manually when we update the packages in Core to ensure the integration works properly.

The issue with running it on each PR is that it's very slow as is: 40mn while in Gutenberg it's split into 4 jobs of 16mn and I'm not sure if we can do the splitting somehow in Core too.

The other concern too is that I suspect that potential intermittent failures might annoy folks.

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


4 years ago

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


4 years ago

#34 @antpb
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50067:

Media: Remove caching from filter by date in media library.

Previously, newly uploaded media attachments were missing when filtering media items by date due to lack of cache invalidation.

Props adamsilverstein, teamdnk, afercia, Mista-Flo, joedolson, youknowriad, talldanwp.
Fixes #50025.

#35 @antpb
4 years ago

In 50068:

Media: Remove unused refresh from _requery.

In [50067] props.cache was removed as it was unused. This left a lingering unused refresh in the _requery function.

Props peterwilsoncc.
See #50025.

#36 @antpb
4 years ago

Just to document here, @adamsilverstein I added a change that removed the refresh param as it was only being used in the bool check removed in the latest patch. I added a param doc to explain it was deprecated. Funny enough it was not previously a documented param so in it's removal it was finally documented. 😆

If you could give [50068] a look to make sure no unintended consequences from my change, I'd appreciate it. Thanks for the quick patch!

#39 @adamsilverstein
4 years ago

#52587 was marked as a duplicate.

#40 @Mista-Flo
4 years ago

#52650 was marked as a duplicate.

Note: See TracTickets for help on using tickets.