Closed (fixed)
Project:
Drupal core
Version:
10.3.x-dev
Component:
media system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
13 Feb 2024 at 03:17 UTC
Updated:
20 Apr 2024 at 15:04 UTC
Jump to comment: Most recent
Comments
Comment #4
mstrelan commentedComment #6
sorlov commentedComment #7
smustgrave commentedSeems to have a check failure
Comment #8
sorlov commentedFixed phpstan issue
Comment #9
smustgrave commentedLeft some comments on the MR.
Comment #10
sorlov commentedComment #11
mstrelan commentedStill need to fix the forms key
Comment #12
sorlov commentedComment #13
smustgrave commentedAppears feedback has been addressed
Comment #14
mstrelan commentedThink there is a syntax error in the example.
Comment #15
sorlov commentedComment #16
smustgrave commentedGood catch! Appears typo has been resolved
Comment #17
alexpottI've left some review comments on the MR - it looks as though we have some incorrect information in the docs and maybe some unused parameters - can someone open the requested follow-ups and also make the suggested improvements to the MR. Thanks!
Comment #18
sorlov commentedMade suggested improvements
Comment #20
quietone commentedI reviewed the unresolved threads and found that they have been resolved. The last three were for followups, which I have just created. All tests are passing, so I am setting to NR.
Comment #21
quietone commentedThis is on hold. The work is being done in #3424509: Update MigratePluginManager to include both attribute and annotation classComment #22
quietone commentedSorry for the noise, this is the wrong issue.
Comment #23
smustgrave commentedFeedback/follow ups seem to be addressed
Searched again for @MediaSource all instances appear to be replaced.
Believe this one is good to go.
Comment #24
alexpottI think we need to discuss the providers key. I don't think it should be on the MediaSource attribute it is only meaningful for the oembed plugin and it is set in the dervier anyway - so I think that means we don't need it in the attribute. It would be good to look at how any module providing an ombed integration is doing it.
Comment #25
alexpottIf they follow the example in \Drupal\media\Plugin\media\Source\OEmbed then it will work because that is telling them to do a
example_media_source_info_alterComment #26
quietone commentedI made the suggested changes but this still needs discussion of that idea.
Comment #27
smustgrave commentedAppears feedback has been addressed.
Sure we would of seen it in tests but made sure that remote embeds still work when using the MR.
Comment #28
berdirSeems very strange to me to have a plugin definition key that only a specific implementation (and its derivates) use. Feels like an incorrect use of plugin definitions to me, but I don't have any great ideas on how to change that, certainly not in the scope of this issue. So +1 I guess :)
Comment #29
smustgrave commentedThink it’s worth it to explore alternatives in a follow up?
Comment #30
alexpottSo looking at contrib we need to think about this some more... for example we have https://git.drupalcode.org/project/media_entity_podbean/-/blob/1.0.x/src...
... sorry should have caught this earlier.
I think we need to create an new OEmbedMediaSource attribute that extends our new attribute. I think this will work just fine because we do an instanceof check in \Drupal\Component\Plugin\Discovery\AttributeClassDiscovery::parseClass and an is_a in \Drupal\Component\Annotation\Doctrine\StaticReflectionParser::hasClassAttribute()
Comment #31
sorlov commentedComment #32
smustgrave commentedAppears feedback for a separate oemebd attribute has been added.
Comment #35
alexpottCommitted 7201d4b and pushed to 11.x. Thanks!
Committed 7f69b30 and pushed to 10.3.x. Thanks!