Problem/Motivation

When migrating media using the media_wysiwyg_filter and you're using the "media_embed" plugin it appears to assume the media ids map 1 to 1 from source to destination. It adds a check which always skips looking up the destination ID of migrated media.

Steps to reproduce

I have a migration with a lot of duplicate files on the source site. On the destination, we've written migrations to dedupe that media. So media with id 1234 might be 5678 on the destination since we only keep one of the two files.

The migration maps have the list of sourceids to destids and maps correctly. But the wysiwyg plugin doesn't check the migration map. It uses the oracle media lookup by the original file id. That won't work for any fid that was deduped. It has to go through the migration mapping.

Proposed resolution

Simply removing one of the checks on the "findDestId()" calls that validates we're using uuids vs ids fixes it. Basically always lookup what the destination id is regardless of whether the referencing ID is numeric or UUID based.

Patch to follow.

CommentFileSizeAuthor
#2 3214791-1-checkdestination.patch1021 bytesminoroffense

Comments

minorOffense created an issue. See original summary.

minoroffense’s picture

minoroffense commented
Status: Active » Needs review
StatusFileSize
new 3214791-1-checkdestination.patch1021 bytes

Here's the patch as mentioned. It just removes one of the checks on the if statement. For migrations who's sourceid and destination ids are the same this should have no impact but fixes those that does have a difference.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Component: Documentation » Code

The patch looks good (especially because of tests are still passing), but for first I want to understand what I was doing wrong before 🙃.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Always check the destination id during migration » Always check the destination id during migration: ensure deduping is respected by the MediaWysiwygFilter process plugin

@minorOffense If you could add a failing test to MediaWysiwygFilterTest, this would get committed very quickly 🤓🙏

minoroffense’s picture

minoroffense commented

@huzooka when you set the "media_migration_embed_token_transform_destination_filter_plugin" to "media_embed" it automatically forces the use of uuids in the embed code. Which means the findId call got skipped. And so by skipping that findId you end up using the original source id. And then that lookup can fail if the media id is different in the destination. Removing the check on the type of ID just makes it so it always tries to find the id (even if it ends up coming back the same).

@Wim Leers I'll try my hand at a test(s) for this.

minoroffense’s picture

minoroffense commented

So the test string would be pretty simple, something along the lines of 

      'HTML text with media JSON tokens with different source and destination ids' => [
        'input' => '[[{"fid":"567","view_mode":"default","type":"media"}]]',
        'expected' => '<drupal-media data-entity-type="media" data-entity-uuid="8c6db715-4cb8-4a3d-be5c-7b9a97752053" data-view-mode="default"></drupal-media>',
      ],

But I can't find where to set the migration map data. Like where would I set the lookup for fid "567" to map to a media element with a mid that points to a uuid of 8c6db715-4cb8-4a3d-be5c-7b9a97752053

I looked in the fixtures and I add a new media element there (if that's where the migration does it's source lookup, I assume so). But I'm not sure how to make a destination id that's different than the source id.

Any suggestions?

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

Hi, could you please check whether #3281055: Media entity migrations shouldn't migrate file IDs to media IDs fixes this issue? I was committed to HEAD.