Problem/Motivation

In #3252386: Use PHP attributes instead of doctrine annotations we added support for attribute based plugin discovery.
As part of that issue we converted block and action plugins.

This issue is to convert \Drupal\media\Annotation\MediaSourceplugins to use Attributes.

Proposed resolution

  1. Add a class to represent the new Attribute - Example
  2. Update the plugin manager constructor to include both the attribute and annotation class names - example
  3. Convert all plugins that use the annotation to use the new attribute - example

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3420997

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

larowlan created an issue. See original summary.

DanielVeza made their first commit to this issue’s fork.

DanielVeza opened merge request !6663

mstrelan’s picture

mstrelan commented
Status: Active » Needs work

sorlov made their first commit to this issue’s fork.

sorlov’s picture

sorlov commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems to have a check failure

sorlov’s picture

sorlov commented
Status: Needs work » Needs review

Fixed phpstan issue

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Left some comments on the MR.

sorlov’s picture

sorlov commented
Status: Needs work » Needs review
mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work

Still need to fix the forms key

sorlov’s picture

sorlov commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Appears feedback has been addressed

mstrelan’s picture

mstrelan commented
Status: Reviewed & tested by the community » Needs work

Think there is a syntax error in the example.

sorlov’s picture

sorlov commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Good catch! Appears typo has been resolved

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup

I'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!

sorlov’s picture

sorlov commented

Made suggested improvements

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Status: Needs work » Needs review
Issue tags: -Needs followup

I 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.

quietone’s picture

quietone commented
Status: Needs review » Postponed (maintainer needs more info)

This is on hold. The work is being done in #3424509: Update MigratePluginManager to include both attribute and annotation class

quietone’s picture

quietone commented
Status: Postponed (maintainer needs more info) » Needs review

Sorry for the noise, this is the wrong issue.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback/follow ups seem to be addressed

Searched again for @MediaSource all instances appear to be replaced.

Believe this one is good to go.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I 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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

If 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_alter

quietone’s picture

quietone commented
Status: Needs work » Needs review

I made the suggested changes but this still needs discussion of that idea.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Appears feedback has been addressed.

Sure we would of seen it in tests but made sure that remote embeds still work when using the MR.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Seems 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 :)

smustgrave’s picture

smustgrave commented

Think it’s worth it to explore alternatives in a follow up?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

So 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()

sorlov’s picture

sorlov commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Appears feedback for a separate oemebd attribute has been added.

  • alexpott committed 7f69b30b on 10.3.x 
    Issue #3420997 by sorlov, quietone, DanielVeza, smustgrave, alexpott,...

  • alexpott committed 7201d4bf on 11.x 
    Issue #3420997 by sorlov, quietone, DanielVeza, smustgrave, alexpott,...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 7201d4b and pushed to 11.x. Thanks!
Committed 7f69b30 and pushed to 10.3.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.