@godotislate

Rebased for #50.

fjgarlin → credited godotislate → .

I took a quick look at Webform submission view building/rendering code, and nothing stood out to me, but I haven't had a chance to debug what could be going on. Regardless, I think we can preserve BC and prevent false positives in cases like Webform submissions by making the new recursive rendering protection an opt-in process per entity type. Idea would be like this:

• Create a new entity type handler definition property of recursive_render_protection or similar
• Create a new EntityRecursiveRenderProtection handler class that does the tracking (essentially the #pre_render/#post_render callback methods in MR 4994 would be moved to this class)
• Only add the the pre_render/post_render callbacks in EntityViewBuilder::getBuildDefaults() if the recursive_render_protection handler is defined for the entity type
• In EntityReferenceEntityFormatter::viewElements(), check whether the referenced entity is of an entity type that has the recursive_render_protection handler. If not, keep the 20 count limit in place. Trigger a deprecation that the 20 count limit will go away altogether in Drupal 12
• Add the recursive_render_protection to all core entity type annotations/attributes)
• Create CR and document that contrib/custom entity types should add the recursive_render_protection property to their definitions by Drupal 12
• There should also probably be a way to alter the recursive tracking key, so custom/contrib code could possibly use that instead of implementing/overriding EntityRecursiveRenderProtection class if they need special logic for recursive rendering protection

If this idea is acceptable, I think it'd be best to wait for 📌 Convert entity type discovery to PHP attributes Needs review to go in, so that the handler definition can be added to entity type classes as attributes, instead of annotations, because that MR is such a beast to rebase. But I think I can provide a PoC of this in a couple weeks.

Rebased MR 6780. There's also a question from 🐛 Move I18nQueryTrait from content_translation to migrate_drupal Needs review about how/whether to deprecate I18nQueryTrait (and the classes that use it), given that migrate_drupal will be moved out of core before D12.

Note that I have a proposed alternative approach in MR 8070 that doesn't require the files be moved at all, and I'd appreciate any feedback on whether it's worth going that route.

There was follow up work needed from [#3420983} to make label and category properties required (see #65 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts RTBC ). The thought was to do it in this issue, but if it's out of scope here, I think a new follow up needs to be created to address that.

One thought is that perhaps category does not need to be required, but we can follow the approach in Drupal\Core\Plugin\CategorizingPluginManagerTrait\processDefinitionCategory(), where the fallback category is the provider name.

Updated IS with summary of alternate suggested approach.

For Drupal core MRs, I don't know that marking them as "draft" really matters much, but often on other projects, reviewers are accustomed to skip reviewing draft MRs/PRs, because it's a signifier of WIP product not ready for review or to merge.

FYI, if you are the MR author, you can move it out of draft by clicking on the kebab menu in the upper right and selecting "Mark as ready".

Rebased and resolved merge conflicts.

Got my POC work in MR 8070 to go green. So along with the steps taken in #44 📌 Convert MigrateSource plugin discovery to attributes Postponed , the next thing I did was eliminate the idea of multiple and automated providers altogether in plugin attribute-based discovery and introduce the idea of manually defined dependencies instead. Complete summary looks something like this:

1. Use PhpToken::tokenize() on the plugin class file
2. Loop through the tokens to find the class declaration token
3. Copy all the class file contents before the class declaration to a string
4. Replace usage of "self" or "static" with the original class name in the string
5. Append a random suffix to the class name for uniqueness, and add a class declaration statement and {} to the string
6. Copy the string to a temporary file and include that file
7. Instantiate ReflectionClass on the new temporary class
8. Extract attribute data from reflection
9. Add a "Dependencies" attribute class that has a "modules" array property which is used to defined other modules the plugin class depends on
10. When parsing the class, look for the dependencies attribute first. If it exists, check to make sure all dependencies are installed, and if any dependencies are missing, prevent the plugin info from being saved to file cache
11. Update the default plugin manager to check that all dependencies are installed in findDefinitions()
12. Add the Dependencies attribute to all migrate source plugin classes that have an implicit dependency on migrate_drupal (or other modules)

This does require developers who write plugin classes to manually account for any implicit dependencies by listing them with the attribute, instead of having discovery determine the providers automatically. While this can be bit of a hassle, I don't think this is a major obstacle, because usually you can determine what dependencies need to be declared by checking the use statements.

This is still POC-ish, but putting back into Needs Review to get feedback on whether this is a good approach. If approach looks good, it might make sense to split out the core discovery part of the work to either #2786355: Move multiple provider plugin work from migrate into core's plugin system → or 📌 Investigate possibilities to parse attributes without reflection Active first before continuing here. But it was useful to test first with all the converted Migrate Source plugins here. Beyond that, there are probably some tests that need to be written for the revised discovery as well.

I tried a POC on an alternate approach for attribute discovery in MR 8070.

The idea is that since only the attributes are needed, parsing can be done like this:

1. Use PhpToken::tokenize() on the plugin class file
2. Loop through the tokens to find the class declaration token
3. Copy all the class file contents before the class declaration to a string
4. Append a random suffix to the class name for uniqueness, and add a class declaration statement and {} to the string
5. Copy the string to a temporary file and include that file
6. Instantiate ReflectionClass on the new temporary class
7. Extract attribute data from reflection

So basically, core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php:

<?php

namespace Drupal\block_content\Plugin\migrate\source\d6;

use Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation as D7BlockCustomTranslation;
use Drupal\migrate\Attribute\MigrateSource;

/**
 * Drupal 6 i18n content block translations source from database.
 *
 * For available configuration keys, refer to the parent classes.
 *
 * @see \Drupal\migrate\Plugin\migrate\source\SqlBase
 * @see \Drupal\migrate\Plugin\migrate\source\SourcePluginBase
 */
#[MigrateSource(
  id: 'd6_box_translation',
  source_module: 'i18nblocks',
)]
class BoxTranslation extends D7BlockCustomTranslation {

  /**
   * Drupal 6 table names.
   */
  const CUSTOM_BLOCK_TABLE = 'boxes';
  const I18N_STRING_TABLE = 'i18n_strings';

}

gets copied to /tmp/BoxTranslationadfrghq.php with these contents:

<?php

namespace Drupal\block_content\Plugin\migrate\source\d6;

use Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation as D7BlockCustomTranslation;
use Drupal\migrate\Attribute\MigrateSource;

/**
 * Drupal 6 i18n content block translations source from database.
 *
 * For available configuration keys, refer to the parent classes.
 *
 * @see \Drupal\migrate\Plugin\migrate\source\SqlBase
 * @see \Drupal\migrate\Plugin\migrate\source\SourcePluginBase
 */
#[MigrateSource(
  id: 'd6_box_translation',
  source_module: 'i18nblocks',
)]
class BoxTranslationhhgsagp {}

and reflection is instantiated on Drupal\block_content\Plugin\migrate\source\d6\BoxTranslationhhgsagp, so the attributes can be read, without concern of inheriting from any unknown class, interface, or trait.

This seemed to work OK with some limited local testing, but CI caught a significant error (among some other failures I haven't investigated). For a class like core/modules/system/tests/modules/cron_queue_test/src/Plugin/QueueWorker/CronQueueTestSuspendQueue.php:

<?php

namespace Drupal\cron_queue_test\Plugin\QueueWorker;

use Drupal\Core\Queue\Attribute\QueueWorker;
use Drupal\Core\Queue\QueueWorkerBase;
use Drupal\Core\Queue\SuspendQueueException;
use Drupal\Core\StringTranslation\TranslatableMarkup;

/**
 * A queue worker for testing suspending queue run.
 */
#[QueueWorker(
  id: self::PLUGIN_ID,
  title: new TranslatableMarkup('Suspend queue test'),
  cron: ['time' => 60]
)]
class CronQueueTestSuspendQueue extends QueueWorkerBase {

  /**
   * The plugin ID.
   */
  public const PLUGIN_ID = 'cron_queue_test_suspend';

  /**
   * {@inheritdoc}
   */
  public function processItem($data) {
    if ($data === 'suspend') {
      throw new SuspendQueueException('The queue is broken.');
    }
    // Do nothing otherwise.
  }

}

When the class contents are removed, the constant PLUGIN_ID is undefined, so self::PLUGIN_ID in the attribute causes an error. I'm not sure how to get around this, so I think this approach might need to be abandoned. I thought I'd document it here regardless.

godotislate → changed the visibility of the branch 3421014-alternate-attribute-parsing to hidden.

Added 🐛 Move I18nQueryTrait from content_translation to migrate_drupal Needs review to the remaining tasks for visibility, because it's a blocker for 📌 Convert MigrateSource plugin discovery to attributes Postponed .

Updated per MR feedback.

I have added deprecations to the moved classes and traits. Do deprecations still need tests? Or is phpstan covering that now?

And again, I apologize to @quietone for missing that you already were working on this and I didn't notice.

@quietone I completely missed the assigneed. Many apologies.

This is a blocker for 📌 Convert MigrateSource plugin discovery to attributes Postponed , because instantiating reflection on classes that use I18nQueryTrait when content_translation is uninstalled cause a fatal error.

godotislate → changed the visibility of the branch 3258581-move-contenttranslation-i18nquerytrait to hidden.

Blocked on deprecating I18nQueryTrait and the plugin classes that use it, and moving them to migrate_drupal in 🐛 Move I18nQueryTrait from content_translation to migrate_drupal Needs review .

Updated IS with remaining tasks

I'm also not sure we need an interface if everything must descend from the base Hook class anyway.

I don't have any use cases in mind, but the idea for an interface would be to allow some degree of future proofing, in case the hook attribute class properties need amendment at some point.

It would also allow other hook attribute classes to do some logic in getters and setters instead of directly acting on property values.

Note that I don't feel strongly about this, but I thought I'd mention it in case.

With the possibility of additional/contrib/custom hook classes, it might make sense to add HookInterface and make every relevant attribute class implement that. With the Interface definining some basic setters and getters (setHook(), getHook() etc..

Also, and I haven't thought through the complexities, I wonder if there are any advantages to making Hook attributes support multiple hooks, such as node_update + node_insert in one hook attribute, or form_node_form + form_media_form in one alter hook attribute?

So

#[Hook('node_update')]
#[Hook('node_insert'])]
public function doNodeStuff(...) {}

becomes

#[Hook(['node_update', 'node_insert'])]
public function doNodeStuff(...) {}

or

#[AlterHook('form_node_form')]
#[AlterHook('form_media_form')]
public function alterNodeOrMediaForm(...) {}

#[AlterHook(['form_node_form', 'form_media_form'])]
public function alterNodeOrMediaForm(...) {}

I wonder if there actually might need to be 3 drupal media plugins like CKEditor5's Image plugin - where there is Image that acts as a "glue" plugin for ImageBlock and ImageInline.

Since this is primarily a 10.2 issue, could this get backported? The patch in #2 🐛 Setting empty URL when making embedded media a link in CKEditor5 causes JS errors Fixed should still apply to 10.2.x, or I can make an MR.

Or if we're going to eat exceptions then only do this in the migrate source case for now and open a follow-up issue to fix this in a better way.

Made a change so that the exception is eaten only for migrate source in the attribute automated providers discovery classes. Made an effort to do that while minimizing code duplication, but it's still pretty ugly. I also tried to limit the exception being eaten based on the error message matching "Interface X not found" or "Class X not found."

I can open a follow up for a better general solution if this approach for migrate source looks fine for now. Or maybe that can be part of #2786355: Move multiple provider plugin work from migrate into core's plugin system → ?

Per #17 📌 Convert Layout plugin discovery to attributes Active on 📌 Convert Layout plugin discovery to attributes Active , labels should be added to all existing layout plugin definitions that are missing them (including a new on in Navigation), and deprecation warning triggered for contrib/custom plugins missing labels (and categories, as well. Blocking this issue on 📌 Convert Layout plugin discovery to attributes Active for the Layout attribute class to get in.

LGTM. Changes are well documented with clear comments. Nice work!

Made label and category optional. Added tests that arbitrary properties are being set correctly via YML and PHP attributes.

Okay... do we have test coverage of step 3?

The change here is that previously step 3 was converting all the plugin data back to Layout annotations via AnnotationBridgeDecorator, while now they're converted to Layout attributes via AttributeBridgeDecorator instead. I was assuming that existing test coverage for Layout discovery (mostly in LayoutPluginManagerTest) + Drupal\Tests\Component\Plugin\Discovery\AttributeBridgeDecoratorTest was enough, unless there's something I'm missing.

Also tests are failing due to a new layout in core. We need to consider the new Navigation module - that adds a layout without a label. I think we should consider a BC layer where we add a label for unlabelled layouts and trigger a deprecation.

Should we address that in this issue, or allow label and category to be optional for now and push requiring them back to 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts RTBC ?

Enforcement that `ckeditor5.plugins` is required is taking place in `CkEditor5AspectsIfCkEditor5Plugin` attribute constructor now, instead of in `CKEditor5PluginDefinition::validateCKEditor5Aspects()`, and an `\ArgumentCountError` is thrown.

Similarly, the type check for `drupal.label` is from `DrupalAspectsOfCKEditor5Plugin` attribute constructor typehinting instead of `CKEditor5PluginDefinition::validateDrupalAspects()`, and `\TypeError` is thrown from the constructor param type mismatch.

The exception types and messages are different, but the result is the same. Removed the exception match string because the new exception messages include a folder path string that varies depending on where Drupal files live in the file system.

Should the changes to Discovery files be done in separate issue? Since it'll apply to everything.

That can be done if needed or preferred, but MigrateSource plugins currently are the only type with multiple providers, so the discovery changes really affects those and is a blocker.

Created an MR against 11.x.

This is likely only a 10.2 problem, as mentioned before users will be prevented from entering empty hrefs via the link tool in 10.3+.

Putting this in Needs Review because it is a minimal diff (falsy check becomes a not null check), to see whether this qualifies as the type of issue that doesn't need a test. (Test would require a new test module and Functional JS test.)

Made category required too.

Made the switch to Attribute discovery and got tests to all green.

Re #2 📌 Convert Layout plugin discovery to attributes Active :
The 'label' property is now required, but somewhere along the way category was set to optional. This might worth more consideration, depending on whether the label and category could be set by the deriver, with validation to completed in 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts RTBC .

Since the editor JS is minified, here is a fix-only patch for 10.2.x.

Rebased after tour removal.

godotislate → created an issue.

Created MR.

godotislate → created an issue.

Added a few comments to the MR. The plugin manager getDiscovery() needs to be updated to use attribute discovery classes, similar to the changes needed for 📌 CKEditor5PluginManager: use PHP attributes instead of doctrine annotations Needs work .

Also, once that change is done, other changes might be needed for to pass tests. For example, at least one of the yaml-defined test layout plugins in web/core/tests/Drupal/Tests/Core/Layout/LayoutPluginManagerTest.php looks like this:

module_a_derived_layout:
  deriver: \Drupal\Tests\Core\Layout\LayoutDeriver
  array_based: true

In which case, arguments in attribute constructor would need to be changed to optional, since the deriver will be providing those properties instead.

This actually caused an error on our project when used with Data export phpspreadsheet → : Error: Call to undefined method PhpOffice\PhpSpreadsheet\Worksheet\Worksheet::setCellValueByColumnAndRow() in Drupal\views_data_export\Plugin\views\display\DataExport::processBatch() (line 838 of {...}/modules/contrib/views_data_export/src/Plugin/views/display/DataExport.php).

That module has a composer dependency on "phpoffice/phpspreadsheet": "^1 || ^2". View Data Export does not have a dependency on that library. It does have a dev-dependency on Excel Serialization → , which has a dependency on "phpoffice/phpspreadsheet": "^1.26", but dev-dependencies of a dependency are not loaded by composer for the root project. In any case, there no constraints against loading 2.x of phpoffice/phpspreadsheet, where Worksheet::setCellValueByColumnAndRow() has been removed.

Added a suggestion to the MR for a more minimal diff.

Tests are green again, and added test for attribute-based plugin.

Swapped out AnnotationDiscovery for AttributeDiscoveryWithAnnotations and AnnotationBridgeDecorator for AttributeBridgeDecorator. With these changes, tests in Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest are failing. Some of that is due to the new CKEditor5AspectsOfCKEditor5Plugin and DrupalAspectsOfCKEditor5Plugin taking named parameters in their constructors vs a values array in their respective annotation classes. The validation tests in CKEditor5PluginManagerTest are failing because there are intentionally arguments missing from the constructors, but it is generating a different kind of exception.

Also, CKEditor5PluginManagerTest needs a sample plugin class using attributes.

godotislate → made their first commit to this issue’s fork.

Whoops, meant to put back in Needs Review.

Rebased, added deprecation test for I18nQueryTrait.

LGTM.

One note, though, that is apparently unrelated. When I test latest Navigation 1.x-dev (#11b42e3) against latest Drupal core 10.3.x-dev (#e03ae01) with standard profile, the third level menu items become unclickable. They do seem to work fine against core 10.2.5. I haven't checked against core 11.x because currently drush does not have a compatible version.

Uploaded screen recording: https://www.drupal.org/files/issues/2024-04-17/Screen%20Recording%202024... →

I looked quickly through the issue queue and didn't see anything, so if someone else can confirm or reproduce, an new issue can be opened for that.

That's probably just a call to opcache_reset() in the extension system?

Though even without that, I don't think it matters:

- you uninstall module Foo

It'd probably have to be apcu_clear_cache(). That does reduce some of the advantage of using file cache, but modules probably aren't installed all that frequently.

And the issue is on install (I spent a week and a half chasing down a test failure similar to this for migrate source plugins):

• Module foo has a plugin class of a type defined in core
• Plugin class has a plugin extender attribute defined in module bar
• Module bar has never been installed
• Plugin discovery picks up the plugin definition without the bar extension attribute values and stores in the definition data array in file cache
• bar is installed, and plugin discovery picks up the plugin definition data unchanged from file cache

One comment on MR for the wrong variable being used as cacheable dependency.

Las thing: there are no automated tests added for this new functionality. Are tests required?

What needs to be figured out is where we put 3rd party definition data on the EntityType definition object. For array definition plugins we just shove into the array! :D

Nice! Though one additional thing that needs to be solved is to delete the plugin class data from APCU file cache when modules are installed.

That being said, I am +1 with @berdir and @longwave to getting this in as near 1:1 as possible so we can move the meta 📌 [meta] Convert all core plugin types to attribute discovery Active forward and close in on deprecation [#326594].

I don't see why we can't have the entity type manager (or indeed any plugin manager) read ALL attributes from plugin classes, and merge the definitions.

Currently attribute discovery only works with one attribute class passed from the the plugin manager, though subclasses are allowed. Adapting for multiple attribute classes would be non-trivial.

protected function parseClass(string $class, \SplFileInfo $fileinfo): array {
    // @todo Consider performance improvements over using reflection.
    // @see https://www.drupal.org/project/drupal/issues/3395260.
    $reflection_class = new \ReflectionClass($class);

    $id = $content = NULL;
    if ($attributes = $reflection_class->getAttributes($this->pluginDefinitionAttributeName, \ReflectionAttribute::IS_INSTANCEOF)) {
      /** @var \Drupal\Component\Plugin\Attribute\AttributeInterface $attribute */
      $attribute = $attributes[0]->newInstance();
      $this->prepareAttributeDefinition($attribute, $class);

      $id = $attribute->getId();
      $content = $attribute->get();
    }
    return ['id' => $id, 'content' => $content];
  }

In addition, if the idea is for something like a FieldableEntityType attribute to live in the field_ui module and not core, then when field_ui is not installed, discovery will hit fatal errors trying to instantiate ReflectionClass on a class having an attribute of an unknown class.

Sorry, missed that the render array cache should be invalidated by changes to the image style. Added comment to MR.

However the conversion to attributes might be the perfect time to make the grouping since people will have to re-write their annotations anyway (so it's not more or less breaking than what we're already doing).

Per @Berdir in #31 📌 Convert entity type discovery to PHP attributes Needs review , it's probably best for this to land ASAP without additional blockers. Though maybe a compromise solution is to add support to set labels per #2813895: Combine entity type labels in an array → now, convert maybe 1 entity type to confirm it works, then do the remainder of the conversion in the other issue? My thinking here would be something like this:

In the constructors of the new attributes, add protected readonly array $labels = [],.

Then in the get() methods of the EntityType attribute:

  $values = array_filter(get_object_vars($this) + [
      'class' => $this->getClass(),
      'provider' => $this->getProvider(),
    ], function ($value, $key) {
      return !($value === NULL && ($key === 'deriver' || $key === 'provider' || $key == 'entity_type_class'));
    }, ARRAY_FILTER_USE_BOTH);

  $label_map = [
    'label' => 'default',
    'label_singular' => 'singular',
    'label_plural' => 'plural',
    'label_count' => 'count' ,
    'bundle_label' => 'bundle',
    'group_label' => 'group',
  ];

  foreach ($label_map as $key => $labels_key) {
    if (isset($value['labels'][$labels_key])) {
      $value[$key] = $value['labels'][$labels_key];
    }
  }

  return new $class($values);

Or maybe do the mapping before the filtering so the property order is maintained.

I believe latest review comments have been addressed. Updated IS and putting back for review.

Tests are finally all green. It was the file cache after all. For any plugin extending a class from an uninstalled module, catching the exception and returning NULL values was resulting in the file cache storing NULL as the value for the plugin definition. Since the cache persists as long as the file is not changed, this meant that even installing the relevant modules did not make the plugin definition discoverable. So instead of catching the exception in parseClass, the exception is caught by the calling function and the plugin is skipped from being stored in file cache.

I think this should be ready for review. Unless the deprecation of the I18nQueryTrait needs a test?

It's been a couple days since I last took a crack at it, but an update for others who might work on this:

I still haven't found the root cause for the test failures. I have found that certain migration plugins, such as d6_action are found by YamlDirectoryDiscovery in MigrationPluginManager::getDiscovery(), but those plugins are then filtered out by the NoSourcePluginDecorator. This means that the source plugins those migrations are using are not being found.

I've also found that the list of source plugins returned from $definitions = $this->getDiscovery()->getDefinitions(); in MigrateSourcePluginManager::findDefinitions() is unchanged after being filtered by the ProviderFilterDecorator, so it seems that setting multiple providers on the attribute recursively by namespace seems to be working correctly. But I'm not sure why discovery is not picking up the source plugins before that, and I still haven't been able to reproduce the failures locally. Since I run the local tests individually, but CI runs them serially, I had a guess that perhaps there was an issue with the file cache persisting incorrectly between tests, , but that does not seem to be the case.

Some suggestions, mostly code style.

I was wrong about which code is leading to the redirect. It's actually a bit further down in the same method: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migra...

    if ($content_conflicts || $translated_content_conflicts) {
...
    }
    else {
      $this->store->set('step', 'review');
      return $this->redirect('migrate_drupal_ui.upgrade_review');
    }

What is expected is that $translated_content_conflicts should not be empty, but it looks like migration discovery from the previous form (CredentialForm.php) is not finding the same migration plugins on CI as on my local (ddev).

I've pushed some debug code to try to see what's going on, but no conclusions yet.

godotislate → changed the visibility of the branch 3396166-convert-entity-type to hidden.

I did take a look at the browser output yesterday, and what's happening is that on CI, when posting to the ID Conflict form, somehow it's being redirected back to first step. Locally, this redirect isn't happening. I assume the code responsible is this, but I have no idea why: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migra...

    // Get all the data needed for this form.
    $migrations = $this->store->get('migrations');

    // If data is missing or this is the wrong step, start over.
    if (!$migrations || ($this->store->get('step') != 'idconflict')) {
      return $this->restartUpgradeForm();
    }

Made the change to move the trait to the migrate module, and removed all the annotations from the plugin classes in question. I've also added a CR for the trait move.

I'm not sure what's going on, because the functional tests failing in CI are passing on my local.

The functional javascript failures I can reproduce on local, but I'm failing to see how they're related. They're also failing in the same way on my local against 11.x, so I'm not sure what's going on there.

Remaining todo:

• Getting tests passing on CI
• Add deprecation test for the trait?

Made an effort to see if I could at least get tests to pass:

• Restored annotations to the plugins that use I18nQueryTrait
• Add exception handling to AttributeClassDiscovery::parseClass when the ReflectionClass is instantiated. This safely handles classes that extend unknown classes, of which there was at least one: Drupal\field\Plugin\migrate\source\d6\FieldInstancePerViewMode extending Drupal\node\Plugin\migrate\source\d6\ViewModeBase

I'm not sure why some tests are still failing in CI. I've run a couple of them locally and they pass.

In any case, there does need to be a solution for any plugin class that either:

• Uses a Trait from another module that may not be installed
• Implements an Interface from another module that may not be installed
• Extends a class from another module that may not be installed

Handling extends is straightforward, since an exception is thrown in that case, but unknown interfaces and traits lead to fatal errors. Does it make sense to create a separate issue for that?

I am disappointed to see many of these changed to one-liners and I have noticed this on other issues as well.

I received feedback to convert id-only attributes to one-liners on other issues for consistency, so I changed them similarly here. I agree that multi-line attributes with named properties is more readable, but people have made cases the other way. While I do have a preference, I don't feel that strongly about it, but putting coding standards in place is a reasonable next step.

The same issue has come up with MigrateSource plugins using a Trait on from another module: 📌 Convert MigrateSource plugin discovery to attributes Postponed .

Made changes to the migrate source attribute:

• source_module is optional
• minimum_version defaults to NULL instead of '' (this is need to pass tests because there is an isset check)
• fixed some tests to match attribute/definition property order
• Properly filter out plugins based on the providers property (as well as provider, singular)

I believe all the remaining tests failures are occurring because there are source plugins using the Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait from the content_translation module that is not enabled. These are the plugins:

• Drupal\menu_link_content\Plugin\migrate\source\d6\MenuLinkTranslation
• Drupal\menu_link_content\Plugin\migrate\source\d7\MenuLinkTranslation
• Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation
• Drupal\taxonomy\Plugin\migrate\source\d7\TermLocalizedTranslation

An fatal error occurs when Drupal\Component\Plugin\Discovery\AttributeClassDiscovery::parseClass() tries to instantiate a ReflectionClass on these plugin classes using that trait. This is similar to 📌 Hidden dependency on block_content in layout_builder Needs work . Likely there will be need to be an overall solution for plugin classes.

My suggestions were minor and straightforward, so I went ahead and applied them to get into review.

Believe 2 were missed EntityFile and Book.php

Those two were already done in 📌 Update MigratePluginManager to include both attribute and annotation class Fixed
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/...
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/book/...

Re-opened to convert remaining migrate field plugins. Added some comments on the MR.

Re-opened to convert the remaining process plugins.

Added the part of the reverted commit for 📌 Update MigratePluginManager to include both attribute and annotation class Fixed that handled migrate source plugins and automated/multiple providers.

As for what to do about the source_module property in the migrate source attribute? I think there are two options:

• Make source_module an optional property
• Wait for 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work to resolve moving source_module from migrate to migrate_drupal

IMO, we can make source_module an optional property now. There is code in the migrate_drupal module (See Drupal\migrate_drupal\MigrationPluginManager::processDefinition() and ::getEnforcedSourceModuleTags()) that already requires that property on migrate_drupal sources.

Deprecating the property from the attribute can be handled in 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work if needed there. There may also be BC concerns if contrib/custom modules have implemented source plugins that use source_module somehow in a custom way that has nothing to do with migrate_drupal.

Re-opening because the balance of destination plugins outside a couple done in 📌 Update MigratePluginManager to include both attribute and annotation class Fixed still need to be converted. MR here has been rebased and is ready for review.

Applied my own suggestions and rebased.

I think the best way to keep moving forward is to de-scope everything to do with migrate source plugins from this issue and handle that in 📌 Convert MigrateSource plugin discovery to attributes Postponed . The remainder of the reverted commit for this issue can be brought back here.

I've put up MR 7327 for the proposed changes for this issue:

• MigratePluginManager
• Destination, Process, and Field attributes
• Sample destination, process, and field plugins
• Doc block references to destination, process, and field annotation classes

Remaining work to do in in 📌 Convert MigrateSource plugin discovery to attributes Postponed :

• Automated/multiple provider handling
• MigrateSourceManager
• Source plugins
• Source attribute
• Docblock references to source annotations

A lot of the migrate source work will just be restoring the remaining diff from the reverted commit. Along with dealing with the source_module property in the attribute, and then converting the remainder of the plugins.

There are currently 5 source plugins that do not have source_module in their annotations, all in test modules. One plugin, with ID no_source_module<code> is meant to throw an exception for missing source module in <code>Drupal\Tests\migrate_drupal_ui\Functional\SourceProviderTest. Otherwise the source_module in source plugins were optional, except if migrate_drupal is enabled and the migrations are tagged. See Drupal\migrate_drupal\MigrationPluginManager::processDefinition() and ::getEnforcedSourceModuleTags()<code>. Now that <code>source_module is a required property in the MigrateSource attribute from 📌 Update MigratePluginManager to include both attribute and annotation class Fixed , this functionality becomes redundant. Is requiring source_module also a BC break? It's somewhat mitigated, since contrib or custom source plugins missing that property in the annotation can have that property added when they are converted to attributes. But maybe a CR is needed to document the new requirement?

Added deprecation warnings to all the static methods with corresponding tests.

Added test.

Failing test-only commit job: https://git.drupalcode.org/issue/maxlength-3391779/-/jobs/1182561
Fixed job: https://git.drupalcode.org/issue/maxlength-3391779/-/jobs/1182625

Thanks for reviewing.

Separately, I wish that there were a similar way to deal with validation Constraint plugins, since my guess is that there are a lot more contrib and custom validation plugins being written than render element plugins, but since Constraint is a Symfony class, not sure there's a solution.

That seems like a lot of additional files to look at. I think those can be done in the original migration plugin type conversion tickets and reviewed separately.

Rebased and resolved conflict.

Updated CR and MR to 12 for removal version.

Updated the default value detection for image fields to account for field storage settings. It's largely copied from Drupal\image\Plugin\Field\FieldFormatter\ImageFormatterBase::getEntitiesToView(). Also updated tests.

Created CR: https://www.drupal.org/node/3436275 →
MR with updated test for deprecation: https://git.drupalcode.org/project/drupal/-/merge_requests/7195

Addressed most comments. Left questions open about requirements_met functionality for anyone more familiar with that than me.

The issue with contrib module Config default image → appears to be separate from the issue with default image in core. The only relation being they were both surfaced by the fix in 🐛 Field block for empty image field with no default image rendering empty div in Layout Builder Fixed .

Briefly about the contrib module: it allows users to set a default image path with the field formatter. Since the default value on the field item is detected to be empty (no uuid), the field formatter is never invoked. An issue for that module probably needs to be opened to address.

Addressing the core issue here: Image field items can have a default image set at the storage level (so for all instances on the entity type) and at the bundle level. Using a standard 11.x install with Layout Builder, if I edit the User Picture field settings and set a default image on the field storage, then the image does not appear. However, if I set the default image on the instance (lower down on the form - see screenshot), then image does appear. My assumption is that when $field->getFieldDefinition()->getSetting('default_image')['uuid']) is called, at the very least, the default_image is not properly merged between the field storage setting and the field instance setting.

TrustedCallback attribute removed.

Updated to one line attributes

I worked on this MR, but if the only outstanding thing was to fix the alias names, I am going to move this to RTBC, since @sorlov's changes address that.

I put in an MR for a change that worked for me:
Testing steps:
1. Installed Drupal core with standard profile (10.3.x)
2. Installed MaxLength 3.0.0-beta1
3. Configured article content type's body field widget to have a maxlength
4. Created a new article node
5. Typed some text into body field (format initially Basic HTML) and confirmed counter matches typing
6. Switched formats to Full HTML and confirmed via dialog
7. Resumed typing and confirmed counter still matches characters typed.

The fix is slightly ugly with the use of the MutationObserver. I think it can be cleaned up once ✨ Trigger event when Text Editor is attached Needs work in Drupal core is in, and there's event that can be listened for instead.

Going to put in needs review for feedback, though it probably needs tests.

godotislate → made their first commit to this issue’s fork.

Added the test for backwards compatibility for annotated migrate source plugins.

Changed admin_label to required and updated docblock.

Do we need additional test coverage, or are the existing tests good enough now that we have converted a few plugins?

I think we should at least add a test to confirm that the system works with a combination of annotation and attribute plugins. Once we convert Drupal core to attributes, there will still be contrib modules using annotations.

This thought did occur to me. We think things are fine now, because in the MR we have a few plugins converted to attributes and most of the rest left with annotations, with all tests passing. But once all the core annotations are removed, annotation support for migration multiple providers might not be directly tested anymore. Looks like for other plugin types there is Drupal\Tests\Component\Annotation\AnnotatedClass\DiscoveryTest, so we'll need something similar for migration source plugins.