#50025 closed defect (bug) (fixed)
Media Library not showing new uploads when filtering by date
Reported by: | teamdnk | Owned by: | 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:
- Use grid mode in Media Library
- Filter by a date
- Upload a new image
- 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)
Change History (42)
#2
@
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
@
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
@
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
@
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
@
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
@
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.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
4 years ago
#11
@
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
@
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
@
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
Trac ticket: https://core.trac.wordpress.org/ticket/50025
#16
@
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
@
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
@
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
#23
@
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:
- Open the post editor
- Add a classic block
- 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.
#26
@
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
@
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
@
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
@
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
@
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
#36
@
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!
hellofromtonya commented on PR #933:
4 years ago
#37
Ticket closed with changeset https://core.trac.wordpress.org/changeset/50067
hellofromtonya commented on PR #809:
4 years ago
#38
Ticket closed with changeset https://core.trac.wordpress.org/changeset/50067
@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?
From what I can tell, based on my testing:
Media Library (grid mode)
Media Modal with the Classic Editor
Media Modal with the Block Editor
It's a bit more complicated to test because different blocks use different media modal types. For example:
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.