Prevent enabling the "media" module if "media_entity" contrib 1.x is already enabled

This makes sense, I think we should have a test for this as well. Leaving 'Needs review' to get more opinions.

Thanks @marcoscano. I talked about this with @larowlan, @catch, and @cilefen, and we all agreed this is a good idea in principle. Core doesn't normally "know" about contrib modules, but in this case this is a small and harmless change that will save a lot of pain for media users.

I'd suggest adding this before 8.4.x, even when the module is not shown in the UI, since it will also help with the problem of sites that have some modules ported to the core API but not others. If they update one module and it tries to enable Media for them, they will get this error.

Based on that, I suggest a few things:

1. We should link the media_entity project page in the message.
2. We should update the media_entity project page to describe the status of the module for 8.4.x and link the relevant core issues.
3. We should link (and create if needed) a handbook page that explains the contrib-to-core transition in more detail. The handbook page should probably link #2860796: Plan for contributed modules with Media Entity API in core so that people can also track the current status without us having to update it all the time.

www.drupal.org/docs/8/core/modules/media is basically empty, so there's some work to be done there. :P I'll add that to the roadmap for "before the module is shown in the UI".

Core doesn't normally "know" about contrib modules, but in this case this is a small and harmless change that will save a lot of pain for media users.

I know that this caused a lot of frustration/WTFs for the sites using BigPipe, when BigPipe was added to core. So +1 to taking the pragmatic approach here :)

I think the key thing that's missing here, is test coverage. Basically, a test that proves that when media_entity 8.x-1.x is installed, this error is shown, and when 8.x-2.x is installed, it is not shown.

And how do we test that exactly? We can't mock system_get_info() nor unit-test a requirements hook, and while we could have a media_entity test module, that would only be one version and might be weird if media_entity is also on the system.

Ugh :(

By looking at the code, I guess we can test it by prepopulating a particular cache item:

function system_get_info($type, $name = NULL) {
  if ($type == 'module') {
    $info = &drupal_static(__FUNCTION__);
    if (!isset($info)) {
      if ($cache = \Drupal::cache()->get('system.module.info')) {

But that's obviously pretty brittle.

I defer the decision whether this needs test coverage or not to the committers then.

That's an interesting idea, we could possibly mess with the static cache there, true.

Agreed, lets defer this decision to committers then, should we assign/tag the issue or mark it as RTBC? Not sure if there's something else left to do here?

We have #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList which would break the static cache trickery but might make it possible to mock the underlying service in a kernel test (unit is not an option because this is a function as I mentioned). So if we do something with the static cache here, we'll have to update it in that issue.

Updated Media entity project page with #7 + few minor fixes. It would be great if any native speakers could review and correct the text. Huge thanks to @marcoscano for this!

I will RTBC this as it looks OK in general. I agree that we wait on committer feedback re tests.

Also moving it back to 8.4. I am aware that this could be tricky, but it should not interfere with any other system and that the benefit of this would likely be significantly diminished if we would need to wait until 8.5. Also citing @Berdir:

this is almost a bugfix imho

:)

Looks like xjm didn't get to marking this fixed, so I will :)

I'm also OK with no test coverage here, it's well outside the critical path and a messy system to test.

Before: Flame! Death! Destruction!
After:

Module media doesn't meet the requirements to be enabled.            [error]
The Media module is not compatible with contrib Media Entity 1.x     [error]
branch. Please check the 2.x branch of that module for an upgrade
path.

Committed and pushed to 8.5.x. I also backported this to 8.4.x since it will prevent painful upgrades around 8.4.0. Thanks!

This is something media users need to be aware of so we should add it to the release notes. No test needed.