Add update path of media_entity config changes from 1.x to core media module

Hi! I can't figure out how to get the 2.x branch via composer :-/ Do I just have to do tarball or git clone for now, or?

Also, is media_entity 8.x-2.x compatible with entity 8.x-1.x?

Thank you!

As of today, the upgrade path / 2.x branch of Media Entity is not fully implemented or ready for testing. There will probably be a patch on this issue when there is something ready to be tested, using the standard drupal patch workflow.

For now, if you want to build something with Media Entity, it is safe to use the 1.x branch. In Drupal 8.3.x there is no confusion, and if you want to test it with Drupal 8.4.x, you can do it too, but in this case you shouldn't enable the core "Media" module at the same time. Once the upgrade path is ready, you will be able to upgrade Media Entity to the 2.x branch, run the updates, then disable the module and start using the core Media solution.

Keep an eye on this issue, as well as on #2860796: Plan for contributed modules with Media Entity API in core and https://www.drupal.org/node/2825215#followup-roadmap , to have a better picture of the current roadmap.

Hi, thanks for the quick reply, @marcoscano! That's really helpful information, thank you for explaining. The only other thing I'd add is, I think the info on the project page gives the wrong idea:

If your site currently uses Media Entity, in order to move to the Drupal core Media solution, you need to upgrade to Media Entity 8.x-2.x first and then follow the upgrade path. More information on the upgrade path can be found in the related issue.

I think even if you copied over what you said in your comment to me here, or change to some variety of "future tense" in the project page info, so it doesn't sound like y'all intend for users to be upgrading to Media 2.x rn, if that makes sense. Something like......

The upgrade path for current Media entity users to move to the Drupal core Media solution is not ready yet, but it will involve first upgrading to Media entity 8.x-2.x and then moving to the core media module. More information about the upgrade path... [etc]

(and then carry on with the "Note that in order to start..." and "If you need to upgrade Drupal core to 8.4.x..." paragraphs.)

Just a suggestion. Thanks again for your quick and informative reply!

Yeah, sorry about the confusion... Wording is always hard :)

Media in core in 8.4.x is really not meant to be "end-user-ready". In the roadmap the upgrade path (this issue) is listed as a must-have before shown on the UI, and this milestone is currently targeted for 8.5.x. In other words, there will be an upgrade path before the Media module is shown on the UI in core 8.5.x. So if the project page info gives the impression that "hey users you need to install core 8.4.x + media_entity 2.x right now!" the text is not doing a good job at all... :)

I've tweaked it a little bit to try to make this idea more evident. I hope it is somehow clearer now.

Oy vey, so hard! :) Thanks for tweaking, looks good!

I was wanting to start working on this a bit. Can we all agree on the approach and make sure we are not leaving any detail behind?

Scenario:
Site with core 8.3.x and Media Entity 1.x + a bunch of plugins. The steps to upgrade will then be:

1. Upgrade core to 8.4.x
2. Download Media Entity 2.x
3. For each provider you have, download its 2.x branch
4. Run Media Entity update hooks, which will:
   • Abort with a big red scary error message if a provider is detected without 2.x branch.
   • Abort if any enabled module or theme depends on media_entity
   • Enable core Media
   • Change module dependencies: replace media_entity by media
   • Move media_entity.settings into media.settings and delete the original
   • For each type (bundle) config entity available:
     • Replace media_entity.bundle.* by media.type.* (using "file" instead of "document" if it exists)
     • Change config dependencies: replace media_entity.bundle.* by media.type.*
   • Fix permissions in roles ("administer media bundles" -> "administer media types")
   • ...
5. Manually disable Media Entity and remove it from the codebase. Remove also Media Entity Image / Document, if present.

Step 4 is basically what the initial approach is doing, and I think this is where we would benefit mostly from additional eyes.

Questions:
- Would provider plugins possibly need additional update hooks on their own, for some reason?
- Should we try to disable Media Entity (step 5) at the end of the update hook to prevent people missing this step, or will the 2.x branch be an empty module only with the update hook? If it's the latter, wouldn't things break if trying to use the site before performing the updates? (or if the update hook aborts early)

Generally looks OK.

Would provider plugins possibly need additional update hooks on their own, for some reason?

Yes, if they need them they should implement them I guess. We'll have to consider requirements (i.e. in which order hooks execute), but I'd discuss for every specific case separately.

Should we try to disable Media Entity (step 5) at the end of the update hook to prevent people missing this step, or will the 2.x branch be an empty module only with the update hook? If it's the latter, wouldn't things break if trying to use the site before performing the updates? (or if the update hook aborts early)

I think that the 2.x will be only update hooks so we could leave it enabled. Maybe display a message in status report that the udpates were run and the module can be disabled?

Here is a first approach to the upgrade patch, based on @chrfritsch's and @phenaproxima's work.

This has not been exhaustively tested. I plan to do more testing in the coming days. However, it would be nice if we could start getting reviews, to spot all possible edge-case scenarios we need to deal with.

The steps to upgrade would then currently be:

1. Upgrade your codebase with:
   • Core: >= 8.4.x
   • Media Entity: 8.x-2.x
   • All media entity providers: 8.x-2.x
   • All modules that depend on Media Entity: 8.x-2.x
2. Run the database updates. This will:
   • Abort if a provider is detected without 2.x branch.
   • Abort if any enabled module or theme depends on media_entity
   • Enable core Media
   • Change module dependencies in config entities: replace media_entity by media
   • Move media_entity.settings into media.settings and delete the original
   • For each type (bundle) config entity available:
     • Replace media_entity.bundle.* by media.type.* (using "file" instead of "document" if it exists)
     • Change config dependencies: replace media_entity.bundle.* by media.type.*
     • Update type and type_configuration with source and source_configuration in all of them.
   • Fix permissions in roles: Replace "administer media bundles" by "administer media types" and revoke instances of the deleted "access media overview"
   • Uninstall media_entity_image and media_entity_document, if present.
3. Manually uninstall Media Entity and remove it from the codebase. Remove also Media Entity Image / Document, if present.

Some questions appeared:

1) About the simple config media.settings update:
The previous code was doing:

  $config_factory->getEditable('media.settings')->setData($config_factory->get('media_entity.settings')->getRawData())->save();
  $config_factory->getEditable('media_entity.settings')->delete();

but I think we can't really do that because the new key for icon_base is icon_base_uri. Is it OK if we just update that key value?

2) system_get_info('module', $module_name) appears to return an empty version for -dev modules. Is there a workaround?

This is the same patch as #9, but here I diff'ed only the media_entity.install file, in order to make a patch of a reasonable size.

1. +++ b/media_entity.install
   @@ -23,6 +26,82 @@ function _media_entity_check_entity_version() {
   +  $incompatible_modules = [];
   +  foreach (array_keys($module_list) as $module_name) {
   +    $info = system_get_info('module', $module_name);
   +    $is_provider = preg_match('/' . implode("|", $provider_modules) . '/', $module_name);
   +    // Modules in our $provider_modules array need to be updated, and all other
   +    // enabled modules shouldn't depend on "media_entity".
   +    if ($is_provider) {
   +      if (version_compare($info['version'], '8.x-2') < 0) {
   +        // @TODO We are assuming here that ALL modules' new branches will be
   +        // 2.x, which may not be the case. If in the $affected_modules we
   +        // included any module where the new branch is 3.x, 4.x, etc, we need
   +        // to treat it as a special case here.
   +        $incompatible_modules['providers'][] = $module_name;
   +      }
   +    }
   +    else {
   

   I'm not sure if we really want to go after the version, won't help with custom providers/modules anyway that don't follow the pattern.

   Instead, my proposal would be to simply load the media bundle configs and make sure it exists as a plugin using the new media.module plugin manager. If not, then we display an error and abort. Maybe this will result in error messages that are not quite as clear (plugin X is missing instead of module X is not updated) but we're much more flexible.

   The current logic would require that even new modules or modules that have no stable version yet have to start with/switch to 8.x-2.x, but there really isn't a reason to do that. A non-stable version can just do a >= 8.4 dependency on media module and do a new release.

   and unless I'm missing something, it would also require us to do completely empty document/image module branches, just to have them exist. Although not 100% sure if having the 8.x-1.x ones still around could result in some errors.

2. +++ b/media_entity.install
   @@ -101,3 +249,70 @@ function media_entity_update_8003() {
   +    if ($role->hasPermission('access media overview')) {
   +      $role->revokePermission('access media overview')->save();
   +    }
   

   we need to open a core issue about this because removing this permission was simply wrong :)

   views.view.media.yml in core still uses that permission, no idea why it was removed and why our tests are still working (unless we only test with admin-users, it should not be possible to access that page).

Thanks @Berdir for the feedback!

#15.1:

True, comparing versions is definitely not the most robust approach. What if we kept MediaTypeManager only for the sake of loading all definitions, and aborting if there is any module implementing a @MediaType plugin? We risk being too aggressive and aborting even if the module is enabled but no bundle is effectively created, but in the end, I believe this scenario would still be preferable.

#15.2:

Wow, I tried to search in patchzilla (#2831274: Bring Media entity module to core as Media module) where this happened to understand why, but I was unable to figure out when it got removed. Opened #2905213: Bring back "access media overview" permission to discuss that.

Some additional tweaks, still a WIP patch.
It's kind of hard to test and troubleshoot this. Maybe it would be worth preparing a boilerplate packed scenario (code + DB, docker image, or something like this) so people working on this could quickly test and provide feedback?

Anyway, some more things I found:
- We should disregard media_entity_image and media_entity_document in the list of incompatible modules, if we don't want to create empty 2.x versions of them, just to bypass validation

- It's important to clear caches after the code was updated but before running the DB updates, otherwise our @MediaType plugin validation will incorrectly fail. I assume this could be a problem in automated upgrade scripts or when upgrading with drush?

While testing, I could proceed to the update hook, but while running the batch process it failed with an AJAX error where the validation message was present:

Existing Media module detected.
In order to run the Media Entity 2.x upgrade, please uninstall and remove from the codebase the contributed Media module.

This means that at some point during the update batch the requirements are evaluated again, which is a problem for the current approach. .. we may want to re-think how we enforce the requirements in this case.

And just for the record, here again an updated and simplified list of steps the user would need to perform to follow this upgrade path:

1. Upgrade the codebase with:
   • Core: >= 8.4.x
   • Media Entity: 8.x-2.x (this patch)
   • All media entity providers: 8.x-2.x (patches from #2860796: Plan for contributed modules with Media Entity API in core)
   • All modules that depend on Media Entity: 8.x-2.x
2. Clear caches AFTER the code has been updated, but BEFORE running the DB updates
3. Run the DB Updates
4. Manually uninstall Media Entity and remove it from the codebase. Remove also Media Entity Image / Document, if present.

Unfortunately, I won't be able to keep working on this in the near future, so if anyone else wants to pick it up from here, please feel free to do so.

We may have a bigger problem to solve here: is it true that drush updb bypasses hook_requirements()?

I can't find any call to the requirements enforcing in https://github.com/drush-ops/drush/blob/4ceda41bedfac4936410c9ddd0f9c508..., the same way Drupal core does when running the updates through update.php on the UI.

If that's the case, it will be a problem for people using drush / automated scripts to update their sites. I believe the hook_requirements is the most important part of this patch, because there we try to ensure that people won't start the DB changes unless they have an environment that meets all conditions to complete the updates. If we can't find a way to enforce that, we risk updates leaving sites half-broken due to some missing requirements (such as some provider modules not available, etc).

I know this issue is for the "Media Entity" upgrade steps, but I think it can't hurt to point out the relationship to the "Media" module in the upgrade notes (as shown here https://www.drupal.org/node/2880334#comment-12238747)
As far as I know, the "Media" module will be in core, and the contrib module "Media" must be disabled before upgrading to 8.4.
If this is correct, I think this should be in the upgrade notes, or the notes should include a link to the upgrade notes for "Media"

Here is the PR to enforce hook_requirements() when running updates with drush:
https://github.com/drush-ops/drush/pull/2708

Nothing like brainstorming in the same room! :)
We found out a way of working around the drush limitation in drush updb (thanks @phenaproxima!), and this patch accounts for that.
We also provide here an additional command to dry-run the requirements and check if your site is OK to upgrade or not.

Fixed the following:

• Correct update hook numbering (minor nit)
• Changing the DB column names for the revision fields since they were changed
• Delete the auto created core file/image media types, we don't want them for existing sites
• Create source fields for media types that don't have one (this shouldn't have been possible in the first place, but that's another story I guess)
• Ported the 'Generic media' source

Me and marcoscano talked about moving the generic media source to a separate module. This will allow people that use this to also delete the media entity module after the update (if you don't use it that is not necessary of course). We think deleting the module is not only important to write a consistent manual, but it is also an important last step. To create a feel-good moment.

The best way to do this might be to already create the module and force people to update in the 1.x branch. The 2.x branch could then check if the 1.x branch was on the right version before proceeding.

@seanB++

This is looking good.

I believe now is the moment to start some exhaustive testing, so I have started to define some test cases:

https://docs.google.com/spreadsheets/d/1q1BM2KbLwQvEDerSN9oxBz82mIYfSBmG...

to help us make sure we don't leave any important one behind.

Also updated the IS that needed some love.

Added a draft for upgrade instructions to the IS.

We have discussed moving the generic media source to a separate module. This way the upgrade path becomes a little more consistent. Everybody can delete media_entity contrib after the upgrade.

TODO: We should create a separate module and make sure the requirement hooks check this when someone is using the generic source.

Marking needs work for this.

Here's a version of the patch without the Generic source, and a separate module containing it.

Still needs work to adjust hook_requirements() accordingly; not sure exactly how to approach that, though.

Added the hook_requirements() check.

Looking good, think there are a couple of things that still need work:
- Create the media_entity_generic module
- I think we should also make sure the update hook installs the media_entity_generic module if needed
- Update the drush command media-entity-check-upgrade to also check if media_entity_generic is needed/available

The check should be in a separate function so we don't have to duplicate the code for requirement/drush/update.

Okay, made the requested changes except for creating the media_entity_generic module. :)

+++ b/media_entity.install
@@ -412,6 +424,16 @@
+    \Drupal::service('module_installer')->install(['media_entity_generic']);

This might need to be done earlier in the update process to make sure the config can be updated, but I will do some manual testing to check this out.

I briefly tested the latest patch this morning, here what I did:
- Site with Media Entity 1.x + Generic, Image and Twitter. One media type of each created.
- Applied the patch, downloaded media_entity_generic and twitter 2.x
- Ran drush mecu to check the requirements.
* Result: ERROR: site cannot be upgraded
- Ran drush cr, and then drush mecu again
* Result: OK: site can be upgraded
- Ran drush updb
* Results:

• The process finished, but with error messages: 'Failed: The "generic" plugin does not exist.'
• Under /admin/structure/media, only the Generic type appears (instead of the 3 that previously existed)
• When trying to edit a media item, I get a WSOD with: "Error: Call to a member function label() on null in Drupal\media\MediaForm->form() (line 23 of /var/www/html/core/modules/media/src/MediaForm.php)"

I can't troubleshoot right now, but wanted to post the result anyways. If I can find more time this weekend I'll resume testing.

Note: I'm uploading a small change on the patch because apparently drush includes .module files of enabled modules, so deleting it completely produces some unwanted warnings. Keeping an empty file solves the problem.

Talked with phenaproxima and we concluded that we need some kind of automated tests for this.

We decided to implement and UpdatePath test and provide a dump containing:

• File and image media types and items
• Articles containing this media items
• A homepage view to show the article

If this is done, we can provide other dumps, to test more sophisticated use-cases.

@phenaproxima will start to work on this.

I have a basic test of the update path working perfectly in my local environment.

However, I would like this patchzilla to land in the 8.x-2.x branch before proceeding, since we know from personal experience that patchzillas are a Very Bad Time.

In discussion with @chr.fritsch, we agreed to commit the patchzilla in #33 in the name of cleanup (and sanity), and continue working on the update path here.

Blocking on #2915251: Create database fixture for testing the update path, which is necessary for us to write automated update path tests.

#2915251: Create database fixture for testing the update path is in, so this is no longer blocked.

Here we go: a passing, basic update path test!

Let's add more!

Added asserts of media linked from the front page view.

This will fail too, since it only applies against the 8.x-2.x branch, which is hidden for the time being.

After discussion in IRC, we agreed to commit this basic update path test, and add more tests and scenarios in follow-up issues.

Great start!

Created follow-up: #2915738: Increase reliability of upgrade path to Media in core

Removed the obsolete upgrade instructions from the IS, pointing interested people to #2917099: Document upgrade path to Media in core instead.