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.

CommentFileSizeAuthor
#85 2981105-85.patch35.15 KBphenaproxima
#80 interdiff-2981105-78-80.txt2.53 KBphenaproxima
#80 2981105-80.patch35.17 KBphenaproxima
#78 interdiff-2981105-75-78.txt329 bytesJeroenT
#78 2981105-78.patch34.75 KBJeroenT
#75 interdiff-2981105-72-75.txt6.99 KBphenaproxima
#75 2981105-75.patch34.29 KBphenaproxima
#74 interdiff-2981105-72-74.txt5.95 KBphenaproxima
#74 2981105-74.patch34.22 KBphenaproxima
#72 interdiff-2981105-65-72.txt24.85 KBphenaproxima
#72 2981105-72.patch29.96 KBphenaproxima
#71 2981105-71.patch31.68 KBphenaproxima
#65 2981105-65.patch4.47 KBphenaproxima
#51 interdiff-2981105-49-51.txt675 bytesphenaproxima
#51 2981105-51.patch15.88 KBphenaproxima
#49 interdiff-2981105-46-49.txt10.06 KBphenaproxima
#49 2981105-49.patch15.88 KBphenaproxima
#46 interdiff-2981105-42-46.txt6.1 KBphenaproxima
#46 2981105-46.patch8.88 KBphenaproxima
#46 2981105-46-FAIL.patch4 KBphenaproxima
#42 2981105-42.patch7.34 KBphenaproxima
#42 2981105-42-FAIL.patch2.55 KBphenaproxima
#39 interdiff-2981105-16-39.txt2.79 KBphenaproxima
#39 2981105-39.patch4.88 KBphenaproxima
#31 interdiff-2981105-28-31.txt3.63 KBphenaproxima
#31 2981105-31.patch23.99 KBphenaproxima
#28 interdiff-2981105-24-28.txt1.75 KBphenaproxima
#28 2981105-28.patch21.16 KBphenaproxima
#27 interdiff-2981105-24-27.txt15.88 KBphenaproxima
#27 2981105-27.patch35.53 KBphenaproxima
#25 2981105-25.png197.05 KBphenaproxima
#24 2981105-24.patch21.17 KBphenaproxima
#21 2981105-21-3.png40.13 KBphenaproxima
#21 2981105-21-2.png39.71 KBphenaproxima
#21 2981105-21-1.png45.36 KBphenaproxima
#16 interdiff-2981105-12-16.txt1.73 KBphenaproxima
#16 2981105-16.patch3.57 KBphenaproxima
#12 2981105-12.patch3.58 KBphenaproxima
#11 2981105-11.patch2.88 KBphenaproxima
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Status: Active » Postponed
phenaproxima’s picture

@catch asked the following in #2962110-121: Add the Media Library module to Drupal core:

+++ b/core/modules/media_library/media_library.install
@@ -0,0 +1,49 @@
+
+/**
+ * Implements hook_install().
+ */
+function media_library_install() {
+  // Change the path to the original media view.
+  /** @var \Drupal\views\Entity\View $view */
+  if ($view = View::load('media')) {
+    $display = &$view->getDisplay('media_page_list');
+    if (!empty($display)) {
+      $display['display_options']['path'] = 'admin/content/media-table';
+      unset($display['display_options']['menu']);
+      $view->trustData()->save();
+    }
+  }
+}

Are we ever going to replace this fully, or is this always going to be necessary? I'm wondering why we couldn't just hide the existing tab, add a new tab, and use a different path. Why do we need to use an identical path here? Could be changed in a follow-up but it feels odd.

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.

phenaproxima’s picture

Title: [PP-1] Media library should modify the media view more intelligently » Media library should modify the media view more intelligently
Status: Postponed » Active

Blocker has landed!

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Martijn de Wit’s picture

How 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?

phenaproxima’s picture

Adding #3061228: Undefined index: media_library.page, as I suspect that is a manifestation of this.

xjm’s picture

Title: Media library should modify the media view more intelligently » Media Library can delete user customizations to the media view and therefore should modify the media view more intelligently
Category: Task » Bug report
Priority: Normal » Critical

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

xjm’s picture

@phenaproxima and I discussed that this is a must-have for the initiative and also probably a critical in HEAD.

phenaproxima’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2.88 KB

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

phenaproxima’s picture

I decided to go ahead with using FileStorage, because it's cleaner for sure. Let's see how many tests this breaks.

The last submitted patch, 11: 2981105-11.patch, failed testing. View results

seanB’s picture

+++ b/core/modules/media_library/media_library.install
@@ -34,27 +48,39 @@ function media_library_install() {
+/**
+ * Reads the default configuration of the 'media' view.
+ *
+ * @return array
+ *   The default configuration of the 'media_page_list' display of the 'media'
+ *   view, as shipped in the Media module's optional config.
+ */
+function _media_library_read_default_media_view() {
+  $directory = \Drupal::service('extension.list.module')
+    ->get('media')
+    ->getPath();
+
+  $storage = new FileStorage($directory . '/' . InstallStorage::CONFIG_OPTIONAL_DIRECTORY);
+  return $storage->read('views.view.media')['display']['media_page_list'];

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

Status: Needs review » Needs work

The last submitted patch, 12: 2981105-12.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
3.57 KB
1.73 KB

Oh...heh-heh. Dumb mistake. Let's see how many tests this breaks.

catch’s picture

This looks like it will be less destructive, but I still have the same question from the original issue:

I'm wondering why we couldn't just hide the existing tab, add a new tab, and use a different path. Why do we need to use an identical path here? Could be changed in a follow-up but it feels odd.

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.

seanB’s picture

Issue tags: +Needs manual testing

The 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:

One more question - if they've altered the default menu settings, but just to change something superficial and we maintain those settings, can it break the intention of the install function leaving those intact? i.e. do they end up with it looking broken?

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.

catch’s picture

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

phenaproxima’s picture

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.

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

phenaproxima’s picture

FileSize
45.36 KB
39.71 KB
40.13 KB

Gave 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:

Two media tabs on /admin/content

The "Media" tab looked more or less correct:

The Media tab seems okay

The "Media Library" tab -- which, remember, is the one I had before, but customized -- looks WTFey:

The Media Library tab behaves strangely

phenaproxima’s picture

I'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. WAT

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

seanB’s picture

The 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

  • Copy the menu and path settings from the table display to a new grid display.
  • Use the same defaults from the table display (filters, etc).
  • Change the fields in the grid display to use a view mode.

Uninstall

  • Copy the menu and path settings from the grid display back to the table display.
  • Delete the grid display.

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 :)

phenaproxima’s picture

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

phenaproxima’s picture

FileSize
197.05 KB

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

Configuration dependencies of Media Library

Status: Needs review » Needs work

The last submitted patch, 24: 2981105-24.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
35.53 KB
15.88 KB

Brought low by config schema errors. Let's see how this one does.

phenaproxima’s picture

Oops, sorry, accidentally rolled another patch into this one. Here's the real patch.

The last submitted patch, 27: 2981105-27.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 28: 2981105-28.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
23.99 KB
3.63 KB

Maybe this'll work? It randomly fails for me locally, a lot...but perhaps testbot likes it better.

xjm’s picture

Maybe this'll work? It randomly fails for me locally, a lot...but perhaps testbot likes it better.

🤦‍♀️We really should get #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest in. Try it locally with that patch applied too?

phenaproxima’s picture

Issue summary: View changes

Updating the proposed resolution in the IS.

phenaproxima’s picture

Issue summary: View changes
catch’s picture

Right now the issue summary contradicts itself:

Upon installation, Media Library will create the grid display and add it to the optional media view that ships with the main Media module. This display will inherit any customizations the site builder has made to that view, except for the fields (which will be overridden in the new grid display). At uninstall time, Media Library will remove the grid display.

Note: Per the feedback in #25, it's worth noting that uninstalling Media Library will also delete the entire Media view unless you remove its computed dependency on the media_library view mode, which would be added by this proposed fix. However, we feel this is an acceptable trade-off because it is the way the config system works, and can easily be resolved by the site builder.

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.

phenaproxima’s picture

Crediting @catch and @seanB for helpful discussion-ing in Slack.

phenaproxima’s picture

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

phenaproxima’s picture

Issue tags: +Needs followup

Tagging for follow-ups per #37.

phenaproxima’s picture

An attempt to implement what we agreed on in #37. Let's see what happens!

phenaproxima’s picture

Tagging for IS update after #37.

seanB’s picture

Status: Needs review » Needs work

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

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs manual testing
FileSize
2.55 KB
7.34 KB

Here's a test which proves that customizations to the path and/or menu settings are not blown away. FAIL patch is the interdiff.

The last submitted patch, 42: 2981105-42-FAIL.patch, failed testing. View results

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Just realized the tests don't cover uninstalling, and I botched the patch as well with invalid coverage. Back to NW!

seanB’s picture

  1. +++ b/core/modules/media_library/tests/src/Functional/InstallTest.php
    @@ -0,0 +1,72 @@
    +    $this->drupalGet('/admin/content/all-media-table');
    

    Do we also need to assert '/admin/content/media-table' doesn't exist?

  2. +++ b/core/modules/media_library/tests/src/Functional/InstallTest.php
    @@ -0,0 +1,72 @@
    +    $this->assertSession()->linkExists('A treasure trove of interesting pictures!');
    

    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.

phenaproxima’s picture

Fixed the test coverage, and added uninstall coverage too.

What happens to the media library view in this case? They now both have the same path right?

I need to look into this.

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.

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

seanB’s picture

+++ b/core/modules/media_library/tests/src/Functional/InstallUninstallTest.php
@@ -0,0 +1,105 @@
+    $this->drupalGet('/admin/content/media');
+    $assert_session->statusCodeEquals(200);

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

+++ b/core/modules/media_library/tests/src/Functional/InstallUninstallTest.php
@@ -0,0 +1,105 @@
+  public function testCustomizedMenuSettings() {

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.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Quickly manually tested:

What happens to the media library view in this case? They now both have the same path right?

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.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
15.88 KB
10.06 KB

Here you go, @seanB. I greatly expanded the test coverage and am asserting things at both the config and functional levels.

seanB’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/media_library/tests/src/Functional/InstallUninstallTest.php
@@ -0,0 +1,249 @@
+  public function testCustomizedAdministationPagePath() {

Apart from this small type this looks good to me. Thanks for the extensive coverage! RTBC when tests are green.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
15.88 KB
675 bytes

Fixed the typo. Still needs follow-ups and IS update to be fully committable, though, so kicking back to NR temporarily.

phenaproxima’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Fixing the IS.

phenaproxima’s picture

larowlan’s picture

+++ b/core/modules/media_library/media_library.install
@@ -12,21 +14,53 @@
+      if ($display['display_options']['path'] === $defaults['display_options']['path']) {
+        $display['display_options']['path'] .= '-table';

what if something else already exists at the given path? do we need to handle that case too?

phenaproxima’s picture

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

catch’s picture

Last 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?

phenaproxima’s picture

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?

Briefly 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 and media_library views. (I think we can safely assume they will be heavily modified.)

To this, @catch asked:

so does that mean the uninstall code has to stay in permanently?

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:

And then how would the uninstall know if it's dealing with an old style situation or the new one?

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

phenaproxima’s picture

Now, 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:

  1. Override all sections of the incoming display, and let the site builder sort it out later. This a "brute force" approach, but it is the option I prefer, since IMHO it has the greatest chance of properly maintaining the "status quo". It is trivial for a site builder go edit the view afterwards and de-override certain things if they want to.
  2. We could go section by section and override only those things which deviate from the defaults of the 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.

catch’s picture

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

seanB’s picture

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

phenaproxima’s picture

Making 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:

  1. Removed the install and uninstall hooks from Media Library
  2. Change the path of the grid view to /admin/content/media-grid, but leave the table view at /admin/content/media
  3. Added runtime code to Media Library -- like an alter hook -- to modify the path of the "Media" local task so that it goes to the grid view instead of the table view

This might work, and fix everything in this issue. It would cease to be a data integrity problem. I'll give it a shot.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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

phenaproxima’s picture

I'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:

  1. The 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.
  2. The media view no longer needs to be changed by Media Library in any way, so our install/uninstall hooks disappear.
  3. The local tasks defined by Media Library will change thusly: Table will come first, and link to the entity.media.collection route. Grid will come second, and link to view.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:

  1. If the media view's media_page_list display still has the path admin/content/media-table, it will be changed back to admin/content/media.
  2. If the media_library view's page display still has the path admin/content/media, it will be changed to admin/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.

seanB’s picture

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

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
4.47 KB

Patch with update path, but no tests yet. Let's see what breaks first.

phenaproxima’s picture

Title: Media Library can delete user customizations to the media view and therefore should modify the media view more intelligently » Media Library should not modify the media view

Re-titling to explain the new scope of the issue.

seanB’s picture

This seems to make everything a lot simpler! Some questions.

  1. +++ b/core/modules/media_library/media_library.post_update.php
    @@ -529,3 +529,30 @@ function media_library_post_update_add_buttons_to_page_view() {
    +  $view = Views::getView('media');
    +
    +  if (!$view) {
    +    return;
    

    Do we really want to skip changing the media_library view when the media view for some reason doesn't exist?

  2. +++ b/core/modules/media_library/media_library.post_update.php
    @@ -529,3 +529,30 @@ function media_library_post_update_add_buttons_to_page_view() {
    +    $display['display_options']['path'] = 'admin/content/media';
    

    Should we also put the menu item back? What happens if we don't?

phenaproxima’s picture

Do we really want to skip changing the media_library view when the media view for some reason doesn't exist?

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

Should we also put the menu item back? What happens if we don't?

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.

catch’s picture

#65 looks a lot healthier and as long as it works as expected seems like a good fix to the critical.

Status: Needs review » Needs work

The last submitted patch, 65: 2981105-65.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
31.68 KB

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

phenaproxima’s picture

Whoa, sorry. Screwed up that patch -- review this one instead.

seanB’s picture

Apart from my earlier feedback, here is a quick look at the tests.

  1. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibrarySetAdministrativePageToTableDisplayTest.php
    @@ -0,0 +1,52 @@
    +/**
    + * @group media_library
    

    This could use a description.

  2. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibrarySetAdministrativePageToTableDisplayTest.php
    @@ -0,0 +1,52 @@
    +class MediaLibrarySetAdministrativePageToTableDisplayTest extends UpdatePathTestBase {
    

    Should we also test the following cases:

    • change the path of the media view
    • change the menu settings of the media view
    • change the path of the media_library view
    • change the menu settings of the media library view
  3. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibrarySetAdministrativePageToTableDisplayTest.php
    @@ -0,0 +1,52 @@
    +  public function testSetAdministrativePageToTableDisplay() {
    

    Missing some docs.

  4. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibrarySetAdministrativePageToTableDisplayTest.php
    @@ -0,0 +1,52 @@
    +    $display = &$view->getDisplay('media_page_list');
    ...
    +    $display = &$view->getDisplay('page');
    ...
    +    $display = &$view->getDisplay('media_page_list');
    ...
    +    $display = &$view->getDisplay('page');
    

    Why &view?

phenaproxima’s picture

phenaproxima’s picture

Sorry, missed a few spots.

The last submitted patch, 72: 2981105-72.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 75: 2981105-75.patch, failed testing. View results

JeroenT’s picture

I guess the patch is failing because the route in media_library.links.action.yml should be updated from view.media.media_page_list to view.media_library.page.

seanB’s picture

  1. +++ b/core/modules/media_library/media_library.post_update.php
    @@ -529,3 +529,34 @@ function media_library_post_update_add_buttons_to_page_view() {
    +  $view = Views::getView('media');
    +
    +  if (!$view) {
    +    return;
    +  }
    

    Let's still update the media library view, even if for some reason the media view is not there.

  2. +++ b/core/modules/media_library/media_library.post_update.php
    @@ -529,3 +529,34 @@ function media_library_post_update_add_buttons_to_page_view() {
    +  if ($display && $display['display_options']['path'] === 'admin/content/media-table') {
    +    $display['display_options']['path'] = 'admin/content/media';
    +    $view->storage->save();
    +  }
    

    Manually tested that the view shows up as expected, even without the menu link. As explained in #68 👍.

  3. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibrarySetAdministrativePageToTableDisplayTest.php
    --- a/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    
    +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -98,7 +98,7 @@ public function testAdministrationPage() {
    +    $this->drupalGet('admin/content/media-grid');
    

    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 then clickLink('Grid'), verify the address and we are on the grid view.

phenaproxima’s picture

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Awesome stuff! RTBC assuming it will be green.

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
35.15 KB

And, done. Restoring RTBC once green on all backends, since this is just a simple reroll.

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Went 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!

  • larowlan committed 78a32a4 on 8.8.x
    Issue #2981105 by phenaproxima, JeroenT, seanB, catch: Media Library...
xjm’s picture

Status: Fixed » Needs work
Issue tags: +8.8.0 release notes, +Needs release note

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

seanB’s picture

Issue summary: View changes
Issue tags: -Needs release note
seanB’s picture

Status: Needs work » Needs review

Added a release note to the issue.

xjm’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs change record

Thanks @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!

bnjmnm’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

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

bnjmnm’s picture

Status: Needs review » Fixed

Received confirmation from @xjm that CR/RN are all set, setting back to Fixed.

xjm’s picture

Issue summary: View changes
Status: Fixed » Needs review

The 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?

phenaproxima’s picture

Issue summary: View changes

The 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?

Yes, 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 the media_library view, and the table is part of the media 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.

xjm’s picture

Issue summary: View changes
Status: Needs review » Fixed

Adding in the updated release note after discussion with @phenaproxima. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.