Problem/Motivation
From #2962110-119: Add the Media Library module to Drupal core:
[
media_library.install
is] assuming the original media library view was never customized, but that's not safe. I moved the view to another location, installed media_libray, uninstalled it, and my customization was reverted.
Proposed resolution
Media Library should not modify the media
view at all.
Since the product managers have agreed to linking /admin/content/media to the table display over in #3081587: Multilingual content is shown double in the media library view, any need for Media Library to alter the media
view has evaporated. So let's remove our install and uninstall hooks, and add an update path that intelligently reverts the changes that media_library_install() previously made to the view.
Remaining tasks
Commit it.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
In Drupal 8.6 and 8.7, Media Library would take over the /admin/content/media
path when installed, changing the original administrative table view's path to /admin/content/media-table
and replacing the content at /admin/content/media
with the Media Library grid view. This could lead to data loss if the site had customized the default table view before installing Media Library.
In 8.8, the /admin/content/media
path has been restored to show the table view, as it does without Media Library installed, and user's customizations will not be overwritten. Sites that installed Media Library in 8.6 or 8.7 and left the listing at /admin/content/media-table
will now have that table restored to its original location at /admin/content/media
, with the grid at /admin/content/media-grid
. This fix might break hardcoded links to either view, or require changes to any modules that further customize them.
The same change record on the updates to the default Media Library views listed above has more detail.
Comment | File | Size | Author |
---|---|---|---|
#85 | 2981105-85.patch | 35.15 KB | phenaproxima |
#80 | interdiff-2981105-78-80.txt | 2.53 KB | phenaproxima |
#80 | 2981105-80.patch | 35.17 KB | phenaproxima |
#78 | interdiff-2981105-75-78.txt | 329 bytes | JeroenT |
#78 | 2981105-78.patch | 34.75 KB | JeroenT |
Comments
Comment #2
phenaproximaComment #3
phenaproxima@catch asked the following in #2962110-121: Add the Media Library module to Drupal core:
Copying it here, since it's relevant. Depending how we unify the grid and table view, perhaps it will not be necessary to modify the media view at all.
Comment #4
phenaproximaBlocker has landed!
Comment #7
Martijn de WitHow do other core modules handle this issue, that are using views to display a list of items. Like the core content view or the watchdog view?
Comment #8
phenaproximaAdding #3061228: Undefined index: media_library.page, as I suspect that is a manifestation of this.
Comment #9
xjmWhoa, this is actually really bad if I'm reading it right, because if it's not addressed, we're causing data loss for someone's site-customized view.
Comment #10
xjm@phenaproxima and I discussed that this is a must-have for the initiative and also probably a critical in HEAD.
Comment #11
phenaproximaHere's a possible approach.
Ideally I'd read the default menu options from the media view that ships with the Media module, using an instance of the InstallStorage class or something similar. However, I'm not sure if that is considered kosher. Would love to hear from a framework manager about that.
In the meantime, this patch uses a simple approach that's more in line with what we currently have.
Comment #12
phenaproximaI decided to go ahead with using FileStorage, because it's cleaner for sure. Let's see how many tests this breaks.
Comment #14
seanBI like this! If we ever change the default (even though I don't expect us to ever do it) it would revert to our new defaults. Not whatever the default was before you installed the module. This was the only thing I could think of that might be tricky, but this seems like the best solution to me.
I like this solution better than altering routes/menu's or runtime config overrides.
Let's add some tests and I think this is a great simple improvement.
Comment #16
phenaproximaOh...heh-heh. Dumb mistake. Let's see how many tests this breaks.
Comment #17
catchThis looks like it will be less destructive, but I still have the same question from the original issue:
So something like:
1. Ship a new view, matching exactly what the altered view looks like here.
2. Alter the access on the existing view display route so it doesn't show up anywhere.
So the default view will disappear/reappear when you install/uninstall, but we don't actually make any changes to it.
Comment #18
seanBThe problem with shipping a new view is that any changes made before installing media library need to be copied to the new view, and any changes made while media library is installed need to be copied back to the original view when uninstalling media library. Keeping those 2 views in sync is probably a pain and seems a lot more risky.
A new display on the existing view could solve that, we could then disable the original display or something. This might be confusing to users though. It's hard to explain why a view or display was hidden or disabled. That is also the reason why I personally prefer the current approach.
After talking to catch in slack the following question came up:
That is probably something that needs some more testing. What will the result look like if menu options are changed before media library was installed, or when the menu is changed before media library is uninstalled? This could use some manual testing with screenshots.
Comment #19
catchSo #18 is a good summary of the discussion.
1. We can have more bulletproof data integrity by adding a new display (and views already inherits changes (or not if you override) between displays. However will this confuse people simply because there's now two displays on the view and one of them doesn't work?
2. If someone makes a superficial change to the default display, like changing the menu label, and we don't alter the config, will it lead to things looking 'broken' because this code doesn't run? Same thing with a path change I guess. If it does undermine the reason for the hook_install() in the first place, I'd lean closer towards adding a new display.
Comment #20
phenaproximaOh, that's a good question. I will manually test it to see what happens if that code doesn't run, and post my results here.
Comment #21
phenaproximaGave this a quick test. I wouldn't say the result is "broken", exactly, but it is unacceptably weird.
Background: I installed Standard and modified the page display of the Media view thusly -- I changed the path to /admin/content/media-library, and the name of the menu tab to "Media Library". Here were the results:
/admin/content showed two tabs:
The "Media" tab looked more or less correct:
The "Media Library" tab -- which, remember, is the one I had before, but customized -- looks WTFey:
Comment #22
phenaproximaI've glanced at the code and I'm seeing how awkward it is. Right now, Media Library is essentially trying to "subsume" the
media
view shipped with the Media module...but without actually merging it into the media_library view. So we end up with an extremely strange set-up where there are two paths of the /admin/content/media view -- one with a grid and one with a table -- yet, although you'd expect them to be provided by the same view as two different displays, they are each provided by a different view. WATA solution I'm leaning towards here is to make Media Library, essentially, mind its own damn business. It shouldn't try to modify the Media view; it should just modify its own view. And it should do that, IMHO, by completely copying the Media view's table display into the media_library view as a new page display at install time, then disabling that display of the Media view (or, indeed, the entire view if there are no other displays but the default one). If we do that, then when Media Library is uninstalled, everything will go back to exactly the way it was before Media Library was installed.
Comment #23
seanBThe problem I have with #22 is that on uninstall, all filters, relations etc added while the media library module was installed will get lost. I think it is a great idea to make the grid/table overview 2 displays of the same view, but I think we should do that in the media view. Just because that means we can keep more of the users changes on uninstall.
It could look something like this:
Install
Uninstall
As a bonus followup we could add plugins to the view header to provide the grid/table switch with the new DisplayLink plugin we are already using in the media library widget displays. That way we could get rid of the local tasks :)
Comment #24
phenaproximaOK, did that. It sounds like a good solution to me, honestly, and reduces the overall confusion with our shipped config. Seemed to work for me on install with very cursory testing...
Comment #25
phenaproximaOne thing that this approach brings up, which might not be a problem but is worth talking about, is that this causes the Media view to get a computed dependency on the media_library view mode. So, if Media Library is uninstalled...the entire view is deleted (see the screenshot below). That is in keeping with how the config dependency system works, so I don't know if that's an issue, but it's worth calling out.
Comment #27
phenaproximaBrought low by config schema errors. Let's see how this one does.
Comment #28
phenaproximaOops, sorry, accidentally rolled another patch into this one. Here's the real patch.
Comment #31
phenaproximaMaybe this'll work? It randomly fails for me locally, a lot...but perhaps testbot likes it better.
Comment #32
xjm🤦♀️We really should get #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest in. Try it locally with that patch applied too?
Comment #33
phenaproximaUpdating the proposed resolution in the IS.
Comment #34
phenaproximaComment #35
catchRight now the issue summary contradicts itself:
If we remove the display in uninstall, then does the config system still uninstall the entire media view? Uninstalling the entire media view seems like just as bad (or worse?) data loss than the original bug here. Concerns about having using two separate views were also worried about the media view getting out of sync with the media library view, but again deleting the entire view is worse than it getting out of sync.
So we should confirm what actually happens (phenaproxima indicated that even if we don't delete the view, we'll tell people we're about to, which also seems confusing). To me using two separate views really doesn't seem so bad here - yes people would need to update both if they want them to match, but better than actually deleting things and should be less fragile.
Comment #36
phenaproximaCrediting @catch and @seanB for helpful discussion-ing in Slack.
Comment #37
phenaproximaI discussed this at length with @catch and @seanB in Slack.
First, I confirmed that, with this patch applied, uninstalling Media Library does NOT delete the Media view, even though the config dependency UI claims it will. Upon uninstall, the Media view is restored to its former state. So @seanB had the right idea in saying that this way is preferable for site builders from a simplicity standpoint.
But, on the other hand, having the dependency UI say "I AM GOING TO DELETE A THING", and then not doing it...that's scary as hell, and it looks like an anxiety-inducing data-loss bug to a site builder, even though it's really just a quirk of the system. That's not good. So @catch's point holds water too. And there doesn't seem to be any real way for us to override that behavior.
The best solution here would be to modify Views so that, in @catch's words, it is "less destructive" if a view mode is removed. (This would involve implementing DependentWithRemovalPluginInterface on RenderedEntity -- not a major change at all.) That would allow us to have our cake and eat it too -- we could merge the views and not report scary lies in the uninstall UI -- but it's also out of scope here. And we really would rather not block this bug, which is an actual factual critical in HEAD, on...changing something in Views, which is never quick or easy to do.
So, we will go back to the original solution of adding some if statements and comparing changes, last seen in #16. (To mitigate @catch's concern in #17, we'll only use specific "important" keys, rather than ignore "superficial" ones like label and weight, for the comparison.) Then we'll file two follow-ups: one to improve RenderedEntity's dependency handling, and another one to consolidate the Media view, as we've been doing since @seanB suggested it in #23. (The latter issue will need to be blocked on the former.)
Comment #38
phenaproximaTagging for follow-ups per #37.
Comment #39
phenaproximaAn attempt to implement what we agreed on in #37. Let's see what happens!
Comment #40
phenaproximaTagging for IS update after #37.
Comment #41
seanBI think this is a great improvement over what we had and I think this fixes the immediate issue, but it does show that we should definitely find a better way for the long term. So huge +1 for the followups mentioned in #37.
I think this is ready to add some tests.
Comment #42
phenaproximaHere's a test which proves that customizations to the path and/or menu settings are not blown away. FAIL patch is the interdiff.
Comment #44
phenaproximaJust realized the tests don't cover uninstalling, and I botched the patch as well with invalid coverage. Back to NW!
Comment #45
seanBDo we also need to assert '/admin/content/media-table' doesn't exist?
What happens to the media library view in this case? They now both have the same path right?
Besides these fail tests for changed items on install, I don't see any tests that verify the same conditions for uninstall (so not being able to place the media view back to its original path. Could we add those?
I also don't see any correct install / uninstall tests (so successfully changing the path and changing it back)? I might not be looking correctly, but if we are missing those it would be nice to add that as well.
Comment #46
phenaproximaFixed the test coverage, and added uninstall coverage too.
I need to look into this.
I purposefully didn't do this because, IMHO, the rest of Media Library's tests implicitly assume that the changes were made. For example, \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testAdministrationPage() goes to /admin/content/media and asserts for various selectors that are only present in the grid display of the media library. It completely covers the "successful" modifications, I think. So for this issue, we just need to ensure that the critical bug we're trying to fix here, is fixed. :)
Comment #47
seanBWe assert the admin/content/media page returns a 200 status code, but we technically don't know which view is returning that. By checking a 200 on admin/content/all-media-table we kind of verify that it wasn't the media view, but I think there is added value in also checking the actual view config for both views.
Still wondering what happens to the paths of both views, so it would be great to add asserts for them.
I think we should also assert a successful uninstall where the path and menu item are changed back as expected. For that reason, it might be good to have a test that verifies our happy path of successfully installing and uninstalling, even though the install is implicitly covered in other tests.
Comment #48
phenaproximaQuickly manually tested:
What happens is that the link I had set up on the media view is preserved. When I click it, I go to /admin/content/media-table. The "Grid" local task is available, and if I click it, I go to /admin/content/media. Seems super logical. Granted, that's without having customized the path at all. I'll add some more test coverage for this overall.
Comment #49
phenaproximaHere you go, @seanB. I greatly expanded the test coverage and am asserting things at both the config and functional levels.
Comment #50
seanBApart from this small type this looks good to me. Thanks for the extensive coverage! RTBC when tests are green.
Comment #51
phenaproximaFixed the typo. Still needs follow-ups and IS update to be fully committable, though, so kicking back to NR temporarily.
Comment #52
phenaproximaFixing the IS.
Comment #53
phenaproximaFiled #3085247: RenderedEntity field plugin should be more flexible about view mode dependencies and #3085254: [PP-1] All displays of the administrative media overview should be in a single view.
I think all I's are dotted and T's are crossed, so restoring RTBC per #50.
Comment #54
larowlanwhat if something else already exists at the given path? do we need to handle that case too?
Comment #55
phenaproximaI don't think so, personally; the previous code didn't either, and if this "overwrites" an existing path, fixing it should be as easy as changing an option in the view.
Comment #56
catchLast question is if #3085254: [PP-1] All displays of the administrative media overview should be in a single view gets unblocked, what does the upgrade path look like from the current approach to the new one for existing sites?
Comment #57
phenaproximaBriefly discussed this in Slack with @catch and @seanB.
Sean and I are leaning towards not having an update path. Although we see potential benefits, we're not certain if those potential benefits are worth the complexity of such an update path -- and make no mistake, it could be quite complex because we don't have a reliable way of knowing just how extensively site builders have customized both the
media
andmedia_library
views. (I think we can safely assume they will be heavily modified.)To this, @catch asked:
Yes, that is pretty much what it would mean, since we'd have to handle both the "old situation" (two views) and the "new situation" (one view for administrators). Quoth @catch:
I suppose there are a few ways to deal with this, but the most reliable way that comes to mind is to check for the presence of a particular display ID in the
media
view. So if the view contains a display calledmedia_page_grid
, we would handle the "new situation". If it doesn't, we would handle the "old situation". @seanB felt okay with this proposal as well.Comment #58
phenaproximaNow, all that being said...what if @catch made us have an update path? @seanB and I quickly brainstormed about it in Slack.
Moving a display from one view to another is reasonably easy, but in order to keep things working as they previously did, we'd have to strategically override various parts of the incoming display. That leads us to two choices:
media
view. To normalize comparison of gigantic nested arrays, we could use JSON-serialize them and compare them that way. This is probably a "slicker" approach, but also potentially more fraught with complications and unknown unknowns. Sean suggested this option.So, altogether: @catch, what should we do? These are our options: no update path, an update path with the sledgehammer "override all the things" approach, and an update path with the "smart overrides" approach.
Comment #59
catchSo my overall feeling here is that the only option that doesn't leave permanent technical debt and is also not fragile is #3085254: [PP-1] All displays of the administrative media overview should be in a single view, but I also realise this turns a critical bugfix into a two issue chain including a patch against Views, so I've asked for opinions from other committers.
Comment #60
seanBIn #3081587-42: Multilingual content is shown double in the media library view the suggestion was made to maybe show the table view by default. In that case we would not longer have to do any magic on install/uninstall.
Comment #61
phenaproximaMaking the table view the default may be desirable, but IMHO that's a separate issue. I thought of another way we might be able to accomplish this. What if we:
This might work, and fix everything in this issue. It would cease to be a data integrity problem. I'll give it a shot.
Comment #62
catch#61 is definitely worth a try.
I'm agnostic on the table view stuff (except that it would fix the data integrity problem which is of course good), seems like a product decision.
Comment #63
phenaproximaI'm pleased to report that, over in #3081587: Multilingual content is shown double in the media library view, both product managers agreed to make the table display the default display for the administrative media view. That choice has a major -- but positive! -- impact on this issue.
Essentially, we need to do the following:
page
display of the media_library view will have a new path by default:admin/content/media-grid
. It will not expose a menu item.media
view no longer needs to be changed by Media Library in any way, so our install/uninstall hooks disappear.entity.media.collection
route. Grid will come second, and link toview.media_library.page
.This does, however, necessitate an update path: in order for the local tasks to still make sense, we need to undo the customizations we've previously made to the
media
view. So we propose a similar approach to what we've been previously dancing around in this issue:media
view'smedia_page_list
display still has the pathadmin/content/media-table
, it will be changed back toadmin/content/media
.media_library
view'spage
display still has the pathadmin/content/media
, it will be changed toadmin/content/media-grid
and its menu item will be removed, regardless of how it may have been customized.The good news is, this is a much more straightforward and simple update path than the previous solutions, and it basically puts the final nail in the coffin of any cross-module config modification Media Library is doing.
Comment #64
seanBThanks for looking into this. One final update hook seems reasonable. There is a lot of value in trying to maintain a unified experience moving forward. Is is also relatively easy to change it back.
Comment #65
phenaproximaPatch with update path, but no tests yet. Let's see what breaks first.
Comment #66
phenaproximaRe-titling to explain the new scope of the issue.
Comment #67
seanBThis seems to make everything a lot simpler! Some questions.
Do we really want to skip changing the media_library view when the media view for some reason doesn't exist?
Should we also put the menu item back? What happens if we don't?
Comment #68
phenaproximaI was going to say yes, because it means the site has changed in unanticipated ways and therefore the update path is no longer reliable. But upon further reflection, the modifications we're making to the media_library view are, at least in theory, fairly isolated and shouldn't be problematic.
I don't believe anything will happen, because the menu item is actually coming from
media.links.task.yml
, not the view itself. :) The media view was just overriding it, for some reason. However, this is certainly something that should be tested.Comment #69
catch#65 looks a lot healthier and as long as it works as expected seems like a good fix to the critical.
Comment #71
phenaproximaHere is some basic test coverage. We can expand it to actually do some functional tests, plus a few customization edge cases. This should also fix the failures in #65.
Note that the fixture change was made by checking out Drupal 8.7.2, importing drupal-8.4.0.bare.standard.php.gz and drupal-8.4.0-media_installed.php, then installing Media Library normally and adding the modified
media
view to the fixture. Creating a whole new fixture for this test seemed like overkill.Comment #72
phenaproximaWhoa, sorry. Screwed up that patch -- review this one instead.
Comment #73
seanBApart from my earlier feedback, here is a quick look at the tests.
This could use a description.
Should we also test the following cases:
Missing some docs.
Why
&view
?
Comment #74
phenaproximaHow's this?
Comment #75
phenaproximaSorry, missed a few spots.
Comment #78
JeroenTI guess the patch is failing because the route in
media_library.links.action.yml
should be updated fromview.media.media_page_list
toview.media_library.page
.Comment #79
seanBLet's still update the media library view, even if for some reason the media view is not there.
Manually tested that the view shows up as expected, even without the menu link. As explained in #68 👍.
One thing I would think makes sense to add here, is verify that the default view for
admin/content/media
is indeed the table view.Maybe we could start the overview test by visiting
admin/content/media
, verify we are on the table display, and thenclickLink('Grid')
, verify the address and we are on the grid view.Comment #80
phenaproximaAll fixed!
Comment #81
seanBAwesome stuff! RTBC assuming it will be green.
Comment #82
phenaproximaComment #83
phenaproximaComment #84
phenaproximaNeeds a reroll now that #3049943: Media library should not use js- prefixed CSS classes for styling is in.
Comment #85
phenaproximaAnd, done. Restoring RTBC once green on all backends, since this is just a simple reroll.
Comment #86
larowlanWent through all of this and I think we ended up in a much better spot.
Thanks for pushing back on that @catch
Committed 78a32a4 and pushed to 8.8.x. Thanks!
Comment #88
xjmI think we should probably add a note about this to the 8.8.0 release notes, in case some contrib or custom module out there had expectations about us screwing with the existing media view.
I wondered about this and #3084043: Convert media_library_update_8703() to a post-update hook. Presumably this only affects new sites, so sites that installed Media Library in 8.6 or 8.7 will still have the post-update run to modify the modified view. Wasn't sure I thought of all the possible edgecases, though.
Comment #89
seanBComment #90
seanBAdded a release note to the issue.
Comment #91
xjmThanks @seanB. I'm expanding the release note with an explanation that this will only affect sites that install Media Library for the first time in 8.8.0-alpha1 or later (right?), not sites that already have the module enabled.
I started to add a link to the CR in the release note, but it looks like the wrong CR is actually attached here? Can we either locate the correct one or write a new one? Thanks!
Comment #92
bnjmnmThe change record covers both this issue and #3081587: Multilingual content is shown double in the media library view as they cover similar territory. Item 1) is specific to this issue, and I've added information about the changes that will occur for sites prior to 8.8.0-alpha1 using Media Library. Setting to Needs Review for signoff on the CR.
Comment #93
bnjmnmReceived confirmation from @xjm that CR/RN are all set, setting back to Fixed.
Comment #94
xjmThe release note here needs a little more work to mention the data model change and link the CR. I started to write the update, but then found myself unsure that what I thought was happening was correct, given the post-update hooks. From the patch, it looks like things are changing for sites that have the default view installed. Also, I thought that there would be some altering of paths or something to just directly use Media Library's views, but that does not seem to be the case.
Can anyone clarify?
Comment #95
phenaproximaYes, things do change for existing sites. The post-update is intelligently restoring to them to the settings they had before they installed Media Library, which may have previously mucked around with their
media
view. By "intelligently", I mean that we do NOT change existing sites that have since deviated from the path and menu settings that Media Library originally imposed.See, in ye olden days, we had install and uninstall routines in Media Library. This was because we wanted /admin/content/media to go to the grid view of all media in the site, rather than the table view (which is where it goes if you don't have Media Library installed). That decision caused some serious awkwardness, which is why those install/uninstall routines were there, blithely modifying the
media
view (and potentially causing data loss). The root of the problem is that the grid display and the table display were, and still are, different views! The grid is part of themedia_library
view, and the table is part of themedia
view. So that's why it was so strange -- we were trying to ensure that /admin/content/media path would be the grid, not the table, and the only way to do that was to modify a view that wasn't "ours".Then suddenly, over in #3081587: Multilingual content is shown double in the media library view, the product managers came to their senses one day and decided that it was okay for /admin/content/media to go to the table view, with the grid view being secondary. That removed any need for Media Library to do install-time modifications, so boom -- away goes our hook_install().
But our hook_uninstall() was trickier. If we just flat-out removed it without an update path, sites uninstalling Media Library later would be left in a weird state; their table view would be left at a modified, undiscoverable URL, and the local task for Media under Admin > Content might not show up anymore. That would be very bad, and would look like a data loss to site builders.
So, the post-update basically does what our uninstall hook used to do. But it only does it once, bringing things into line with a clean installation of Media Library, and then that's it. No more mucking around. But I repeat: it only does this for sites that have NOT deviated from the settings Media Library used to impose at install time.
Does that make sense? I adjusted the release notes.
Comment #96
xjmAdding in the updated release note after discussion with @phenaproxima. Thanks!