Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Since D8.6 we have oEmbed support in core. Should we provide an update path from video_embed_field to the core sources?
Comment | File | Size | Author |
---|---|---|---|
#29 | 2997799-29.patch | 14.17 KB | chr.fritsch |
#29 | interdiff-2997799-27-29.txt | 1013 bytes | chr.fritsch |
#27 | interdiff-2997799-25-27.txt | 2.15 KB | chr.fritsch |
#27 | 2997799-27.patch | 14.06 KB | chr.fritsch |
#25 | 2997799-25.patch | 11.91 KB | chr.fritsch |
Comments
Comment #2
marcoscanoI may not have a good picture of what sites out there are using, but from my (likely biased) perspective, I'm not sure media_entity is the best place for this. I have the feeling that most sites that want to use media in core at this point are already using Media in core + VEF 2.x. So I'd say this would either need to be in core (unlikely), or maybe a new branch of VEF that only served to perform the upgrade, and then be disabled?
Comment #3
BerdirAgreed, this belongs into VEF and can't be done here.
Comment #4
seanBI agree with marcos en berdir. If video embed field wants people to move over it should provide an upgrade path. It is basically a migration from 1 media source to another. Since it supports a whole lot of providers migration could be a bit tricky.
Comment #5
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedQuoting from slack. I think this would be a great idea.
Comment #6
chr.fritschI started working on this on my way to DrupalEurope. Hopefully, we can finish it there.
Comment #7
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedLooking like a great start. It would be good to have some tests for this as well.
I didn't realise you could do this. I thought once a field schema was installed, changing the type or storage settings was impossible. After running this, does "drush entity-updates" complain about pending schema updates?
Comment #8
chr.fritschYou are right. drush entup is complaining
Drupal\Core\Entity\Exception\FieldStorageDefinitionUpdateForbiddenException: The SQL storage cannot change the schema for an existing field (field_media_video_embed_field in media entity) with data.
So looks like we have to do something else to change the field definition.
Comment #9
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThere are ways to hack the repository to change the field type, given both VEF and string use the 'value' column, but overall I imagine it's probably easiest to create a new field and copy the data from the old one to the new one.
Comment #10
chr.fritschDiscussed this with @webflo and @alexpott and we think it's best to change the field schema on a very low level.
Comment #11
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis is really nice work, only thing I picked up was below. If possible a test would be useful, otherwise I'm happy to get this in and iterate from there.
We could use \Drupal\Core\Entity\Sql\DefaultTableMapping::getFieldTableName for this, although the hardcoded table name is probably reliable.
Comment #12
BerdirNo, it's actually not because if someone has a long field name, it gets hashed.
Hardcoded Alter Table statements are also MySQL specific, why isn't this using the schema API?
Comment #13
jibranI wish we can write D8 -> D8 migration but @Berdir is right we should be using schema API.
Comment #14
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI believe VEF used to create the field for you automatically when adding a new media entity bundle, with a fixed name, but you're correct it would be more reliable to lookup the table name.
EntityDefinitionUpdateManager doesn't support changing a field type, so I think the only alternative would be #9, unless there is a general field schema api?
Comment #15
BerdirYou can use \Drupal::database()->schema() also on field tables, it doesn't care what kind of table it is. You just tell it which field on which table to change and what the new definition is.
Comment #16
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedGotcha, that sounds preferable. Thanks for clarifying.
Comment #17
chr.fritschThen it should be like this
Comment #18
Nick Hope CreditAttribution: Nick Hope commentedNot sure if I'm a typical VEF user, but oEmbed would need Colorbox support (the same as VEF) for me to migrate to it, unless I can get oEmbeds to display in modals some other way. Others might need one of the the other display formats such as "Video (lazy load)" perhaps?:
Also, I still have 3 issues with oEmbed in 8.6.1, although the lack of response makes me wonder if some of them may be unique to my situation:
Also I guess some may need the field mapping that VEF provides:
Unfortunately I'm not a coder, or I would try and fix/implement this stuff myself. Just raising some considerations that someone working on this might overlook.
Comment #19
chr.fritschIt's true, oEmbed currently doesn't support colorbox. But IMHO that should not block this issue. This is only an optional upgrade path. If you need colorbox or lazy video you can still use this module. That's totally fine.
Comment #20
seanBThe colorbox module contains a patch for media support: #2957030: Allow media entity reference fields to be displayed as a gallery in a colorbox
Comment #21
chr.fritschA new patch that fixes wrong dependencies in the config files after the migration.
Comment #22
chr.fritschOoups, the previous patch was empty
Comment #23
daniel.bosenI tested this patch and the migration went very smooth.
Comment #24
chr.fritschCan we get that committed?
Comment #25
chr.fritschI added drush 9 support
Comment #26
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI'm kind of hesitant to commit this without a test, especially given that it would be great to have proof this works as D9 compatibility approaches. Bumping the major version of core is probably a good time to evaluate all of these dependencies that may be outdated.
Comment #27
chr.fritschOk, so here we go.
It's the first test to check that the source and the field type was changed.
Comment #28
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCool, based on this having a test, being completely optional and in a sub-module I think this is safe to go in.
RTBC, will commit tomorrow if nobody has any further feedback.
Thanks!
Comment #29
chr.fritschUploading a new patch that fixes a wrong dependency on the media type.
Comment #30
m.abdulqader CreditAttribution: m.abdulqader at Sprintive commented+1
Comment #32
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #33
chr.fritschThanks for committing this. Could we get a new VEF release?
Comment #34
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedDone. 2.2 tagged.
Comment #35
ThomWilhelm CreditAttribution: ThomWilhelm commentedWill this migrate video_embed_fields added to nodes? I'm looking at an upgrade to core media at the moment from media_entity 1.x and found this interesting. However from an early test this only covers migrating video_embed_fields added to media entities, but doesn't migrate those added to nodes.
Comment #36
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI don't believe that kind of migration was in scope for this issue, no.
Comment #38
bryanbuchs CreditAttribution: bryanbuchs commentedIs there a usage example for this? I've got a media entity using the video_embed_field, but when I enable this sub-module, clear caches, & run
drush venmo
I get a "command could not be found" message. Tried against my project install running 8.7.6 and the 2.2 release version of video_embed_fieldComment #39
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI have added a documentation page for "Migrating to core media
": https://www.drupal.org/docs/8/modules/video-embed-field/migrating-to-cor....
If anyone has capacity, it would be great if details on how to use the upgrade path committed here was added to this docs page.
Comment #40
rjzaar CreditAttribution: rjzaar commentedi found drush vem:migrate_oembed worked (D8.8). drush venmo or drush vemmo didn't work for me. I found it using drush list, even though in the vem_migrate_oembed module the alias is vemmo.
Comment #41
Robert DeArmond CreditAttribution: Robert DeArmond commentedOn Drupal 8.8.6.
When I run
drush vemmo
I get the following error:
When I run
drush vem:migrate_oembed
I get the following error:
Any advice?
Comment #42
jonathanshawI fleshed out the docs.
Comment #43
drupalbrax CreditAttribution: drupalbrax commentedI get the same error as described in #41
Comment #44
Rob230 CreditAttribution: Rob230 as a volunteer and at CTI Digital commentedThe documentation is wrong. There is no such drush command as
vem:migrate_oembed
in the code that I can see. It's calledvideo-embed-media-migrate-oembed
orvemmo
.But when I run this I get the error:
I don't know if this means some other part of the upgrade process has gone wrong. To me it is not clear what order things need to be done in. I have already upgraded media_entity and all media providers to 2.x and run database updates.
Edit: vem_migrate_oembed is the name of the command in 2.4. I'm having to use 2.2 because later versions require 8.8 which I obviously can't upgrade to yet, given that I'm still doing media updates.
Edit2: The error I got was from badly applied update to media_entity (it didn't complete).
Comment #45
marcoka CreditAttribution: marcoka commentedI am not sure but that migration command will not migrate simple video_embed fields?
At least it did not do anything fpr me using a simple embed field with a youtube url.
Comment #46
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedThis is not fixed, please reopen the issue.
The following did not work:
To migrate the videos, I executed
drush php-eval '\Drupal::service("vem_migrate_oembed.migrate")->migrate();'
isntead.Comment #47
SKAUGHT#46.
drush version could be the problem. @drush9+ commands
Comment #48
rattusrattus CreditAttribution: rattusrattus at Agile Collective commentedIf anyone is looking for an upgrade path from video_embed_field fields attached to nodes etc. then you can check out https://www.drupal.org/project/video_embed_field_migrate