Problem/Motivation

As stated in the roadmap on #2825215: Media initiative: Essentials - first round of improvements in core we need to move the media module out of the Core (Experimental) package and into the Core package, but mark it hidden until must-have issues are completed. This allows stable contrib modules to depend on the API even if the user-facing core features below are not finished.

Proposed resolution

Move the module and mark it hidden.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

seanB created an issue. See original summary.

seanB’s picture

Status: Active » Needs review
FileSize
406 bytes

Here is the patch for it.

Gábor Hojtsy’s picture

Status: Needs review » Postponed

Yay! For now this would be postponed on steps in https://www.drupal.org/node/2825215#followup-roadmap (first list), right?

seanB’s picture

Correct!

xjm’s picture

Status: Postponed » Active

As discussed in #2825215-98: Media initiative: Essentials - first round of improvements in core, the committers have agreed that this is unblocked now and can be unpostponed! The product, framework, and release management teams agreed this on Thursday and it will just need Dries' signoff.

xjm’s picture

Status: Active » Needs review

Ah there is a patch already, so setting back to NR. Thanks!

Berdir’s picture

Assigned: Unassigned » Dries
Status: Needs review » Reviewed & tested by the community

Given that we have a patch and all we need is Dries signoff, is this the right status?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2895059-2.patch, failed testing. View results

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
1.61 KB
1.22 KB

Here's the updated patch. The tests failed b/c the module is hidden in UI for now.

//Naveen

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Media Initiative

Back to RTBC per #7 as this needs sign off from Dries

marcoscano’s picture

+++ b/core/modules/media/tests/src/Functional/MediaInstallTest.php
@@ -28,15 +28,10 @@ protected function setUp() {
     $this->assertSession()->pageTextNotContains('could not be moved/copied because a file by that name already exists in the destination directory');

This test only makes sense when we install using the UI. Now that the module is hidden, maybe we should remove it altogether? (Perhaps with a @TODO to bring it back when the module is exposed again)

Berdir’s picture

Users in contrib will still enable the module in the UI indirectly, through one that depends on it. We can replicate that quite easily by adding a test module that depends on media and enable that.

marcoscano’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
1.89 KB
1.38 KB

True! It turns out we already have test modules we can use for that, thanks!

Setting it back to needs review to have another opinion before RTBC.

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! This accommodates #12

Dries’s picture

+1 from me. Love the progress!

Wim Leers’s picture

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup

I like #13; it tests the way users will actually install the module.

Can we file a followup issue to change this test once Media is shown in the UI and link it in the codebase?

naveenvalecha’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs followup
FileSize
1.95 KB
1.09 KB

Here's the follow-up #2897028: Update the tests once the media module will show in UI and updated the patch accordingly. Back to RTBC

  • xjm committed 828f315 on 8.4.x
    Issue #2895059 by naveenvalecha, marcoscano, seanB, xjm, Berdir: Move...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Thanks @naveenvalecha!

I tweaked this slightly on commit since the @todo didn't quite match the standard formatting from them. Here's the diff on commit vs. #13 (which I used instead of #18 to avoid rewrapping lines on commit):

     // Install the media module again, through a test module that depends on it.
     // Note: We use a test module because in 8.4 the media module is hidden.
-    // Once it gets exposed again, this can be simplified.
+    // @todo Simplify this in https://www.drupal.org/node/2897028 once it's
+    //   shown again.

Congratulations everyone!

naveenvalecha’s picture

Awesome thanks @xjm