@godotislate

Applied suggestions from @quietone.

I haven't read the full thread but I'm a bit confused why we'd want to show a linked username if the user doesn't have access to view the profile? We'd be showing a link to a 403?

No, the username label would not be linked at all. It would just be text.

The effective difference only applies to users who are viewing their own username's label. Access for the "view" operation is allowed for users with the "access user profiles" permission and for users viewing their own profile. Access for the "view linked label" operation is allowed for users with the "access user profiles" permission and ignores whether the user is viewing their own label. This means that the label is displayed as a link to all users, or displayed as text to all users. This removes the need for the user cache context, and it also makes the label display consistent to all users.

Looking at the HTML5 Standard, the maxlength attribute for a form control is validated by its "API Value".

In the case of textarea elements, the API value and value differ. In particular, newline normalization is applied before the maximum allowed value length is checked (whereas the textarea wrapping transformation is not applied).

And the newline normalization converts "\r\n" and "\r" to "\n". So in PHP, the maxlength should be validated against the submitted value with the same newline normalization.

This likely affects submissions regardless of the user's brower/OS. Also per the HTML5 standard, the submitted form values should have "\n" and "\r" normalized to "\r\n". So the form values in PHP should have "\r\n" exclusively. Testing on Chrome in MacOS, I can validate that this is what I've observed. Putting a breakpoint on these lines in Drupal\Core\Form\FormValidator::performRequiredValidation():

    // Verify that the value is not longer than #maxlength.
    if (isset($elements['#maxlength']) && mb_strlen($elements['#value']) > $elements['#maxlength']) {
      $form_state->setError($elements, $this->t('@name cannot be longer than %max characters but is currently %length characters long.', [
        '@name' => empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title'],
        '%max' => $elements['#maxlength'],
        '%length' => mb_strlen($elements['#value']),
      ]));
    }

If I only hit "return" 20 times in a textarea with #maxlength 20 and submit the form, then the value of $elements['#value'] is "\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n", and mb_strlen($elements['#value']) is 40.

I think best way forward is to do the same newline normalization in Drupal\Core\Render\Element\Textarea::valueCallback(), so that the maxlength validation is equivalent on the frontend and backend. I think it also makes sense to store this normalized value, so that there isn't an error if the same character limit is applied on the database column, but it needs to be validated that when the value is retrieved and presented back to browsers, that this doesn't create any issues across browsers/OSes.

Per #6, was there also a regression for '#type' => 'container' that is fixed here? In which case, should there be a test for that as well?

I think this change in DrupalKernel might be the wrong approach:

@@ -1396,7 +1396,7 @@ protected function compileContainer() {
     $container->setParameter('app.root', $this->getAppRoot());
     $container->setParameter('site.path', $this->getSitePath());

-    $container->compile();
+    $container->compile(TRUE);
     return $container;
   }

Looking at the documentation for \Symfony\Component\DependencyInjection\ContainerBuilder::compile(bool $resolveEnvPlaceholders = false):

     * @param bool $resolveEnvPlaceholders Whether %env()% parameters should be resolved using the current
     *                                     env vars or be replaced by uniquely identifiable placeholders.
     *                                     Set to "true" when you want to use the current ContainerBuilder
     *                                     directly, keep to "false" when the container is dumped instead.

Generally the Drupal container does get dumped (and cached), outside of kernel tests. So $resolveEnvPlaceholders should not be set to TRUE per that documentation. In addition, resolving the environment placeholders at compile time would mean that it does not work as Symfony documents:

Use the special syntax %env(ENV_VAR_NAME)% to reference environment variables. The values of these options are resolved at runtime (only once per request, to not impact performance) so you can change the application behavior without having to clear the cache.

Instead, with the placeholders being resolved at compile time, any changes to env variables after the container is cached would not update the container parameter values without a cache rebuild. (If it's fine to support environment variables this way for Drupal, then that difference would need to be documented in the change record.)

And also, as seen in #20 and #28, the extra compile pass resolving the environment variable placeholders is having unexpected side effects.

I think the necessary change to support environment variables is a larger lift: Drupal\Component\DependencyInjection\Container would need to reproduce the environment variable replacement functionality that Symfony\Component\DependencyInjection\Container has, but since the Drupal container is quite different from Symfony's, this effort doesn't look straightforward to me.

Separately, I think changing the Yaml parser to handle constants is out of scope for this issue. Work for that is being done in 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work .

I think the no-needs-review-bot tag deactivates the bot.

IIRC, using \Drupal::service() instead of $this->container->get() addresses issues that surfaced while trying to fix 📌 Memory leak in DrupalKernel when installing modules Active in any test that has a container rebuild. Some of the relevant discussion might've been in Slack, though.

Rebased, removed post update hook per #26. Also autowired the service closure per the original idea in the IS and updated the CR.

Can the work done to this point in the issue fork be turned into an MR (draft, if preferred), to make it the base of any effort going forward?

The tests added for 🐛 Link-widget throws exception when rebuilding a form with an invalid uri Fixed and 🐛 Uncaught exception in link formatter if a link field has malformed data Fixed might be good starting points for testing the logging added here, though perhaps 🐛 Uncaught exception in link formatter if a link field has malformed data Fixed .

Drupal\Tests\language\Kernel\LanguageNegotiatorPluginTest might also provide an example of how to use the BufferingLogger class to assert expected messages were logged.

I am still working on understanding the discussion here, so maybe I do not have it quite right.

Summary of discussion/issue so far:

• The original proposal was to allow any module to provide attribute classes that could add supplementary property values to plugins for plugin types defined by core or other modules. POC of this work is in MR 7793, but the main issue with this approach is that it does not account for the behavior of plugin attribute discovery file cache
• Alternate approach, which is what is currently described in the IS, is for core to provide an attribute class that can be used to set supplementary properties on plugin definitions. This class can be extended in core to apply to plugin types that have object-based definitions (such as entity types) instead of array-based (almost every thing else). The class could also be extended by modules defining the plugin type, but not by other modules. This is done in MR 11218
• One of the plugin subsystem maintainers prefers allowing arbitrary values being set on the plugin attribute itself. This is done in MR 10668. The current small diff might be a little misleading: constructors for every plugin type attribute, or at least, the ones that should allow arbitrary attributes to be set, need to be updated.

@catch has suggested in 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work #70 that the file cache issue could be addressed by making the file cache entry keys include the install modules list. Doing so would make the original idea possible.

I also just thought of another possibility to make the original possible once 📌 Add a fallback classloader that can handle missing traits Active gets in, but I'd need to POC it first.

We haven't done a release with the conversion yet, we could still execute it this way and not worry about it.

I agree this seems to be the most straightforward way ahead in that it isn't dependent on resolving blocker issues on the plugin subsystem as a whole.

However, the cache key is set in AttributeDiscoveryWithAnnotations::getFileCacheSuffix and I think we could key that by rootNamespaces which is iirc more or less the module list, that way it will always depend on the combination of modules enabled.

@catch Any concern about possibly having multiple entries, based on combination of modules installed, for just about every file in the plugin directories for every plugin type? Though this would be mitigated once 📌 [meta] Lots of non-plugin PHP classes in plugin directories Active is addressed.

This could be isolated to migrate source plugin discovery, because it has its own classes AttributeClassDiscoveryAutomatedProviders and AttributeDiscoveryWithAnnotationsAutomatedProviders. On the other hand, it's probably not ideal to make migrate source discovery any more special than it already is.

Getting ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active would be great, but I think it's at an impasse for now on what approach to go forward with.

Another idea would be to remove the source_module property from the MigrateSourceattribute, and use

hook_migrate_source_info_alter()

implementations to add the source_module property to the relevant definitions. This would be similar to the approach taken for the Navigation module in 📌 Provide a way for other modules to flag block plugin implementations as 'navigation safe' Active , where hook_block_alter() implementations can be used to add the allow_in_navigation property to block plugin definitions. For migrate_drupal source plugins, this probably could be done as one single alter hook in migrate_drupal for all the relevant plugins throughout core modules.

I don't think it's necessary to add an other comment for additional services that need to be re-resolved. In that case, only the comment on the test is needed because it's really connected to this behavior.

Sorry if this was unclear, but yes, this is the only comment that was needed. My previous post referred to my suggestion on the MR.

Concerning the documentation, I would agree to add a comment but I don't know where to put it. Does the sites/default/default.services.yml file seem right to you (After that, the content of the documentation is the same as for Symfony)?
I don't think we have access to the technical documentation repository to add a section.

The ask isn't for more documentation in code. You can follow these instructions on how to add a change record to a Drupal issue → . For an actual example, look at the change record documenting autowired services → being added to Drupal.

Then I created the MigrateDrupalSource attribute class, extending MigrateSource, and moved the source_module property to the new class.

The unfortunate issue with an attribute being defined in a module separate from the one that has the the main attribute is this:

• Site has the migrate and my_module modules installed, but not migrate_drupal
• On cold caches, migrate source plugin discovery iterates through all the plugin directories and picks up definitions using the MigrateSource attribute. The MigrateDrupalSource attribute on any class is ignored as a comment, since the attribute class does not exist with the module uninstalled. This all works as expected without error
• However, once discovery iterates through a directory, the definition data for all the files in that directory are saved in a file cache in APCu, and the entries in that file cache are only invalidated by change in file mtime
• So, assuming you have a migrate source plugin Drupal\my_module\Plugin\migrate\source\MyMigrateDrupalSource that uses the
  

  MigrateDrupalSource

  attribute, the entry associated with the class saved in the file cache will be NULL

• If you install migrate_drupal, the file cache data is not invalidated, so the
  

  NULL

  value for MyMigrateDrupalSource is retrieved from cache. MyMigrateDrupalSource.php doesn't get parsed for the attribute data, and the definition is not discovered

There are some attempts to deal with a very similar problem in ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active .

One comment: should document the additional services/parameters that need to be re-resolved, accounting for the additional compiler pass.

Is this being ported to 10.5.x? Just noticed it's been in "Patch (to be ported)" for a couple months now.

Note that this is probably soft-blocked by 📌 Hook ordering across OOP, procedural and with extra types Active , or at least there will likely be a giant merge conflict resolution needed between the two.

So IF $container->registerAttributeForAutoconfiguration is used, i see no rational reason for performance fear left.

So while core does not use $container->registerAttributeForAutoconfiguration, with

autoconfigure: true

and several interfaces registered for autoconfiguraiotn in CoreServiceProvider::register() with $container->registerForAutoconfiguration() (no Attribute), it does turn out that at compile time all core services are iterated over and reflected.

Given this, @ghost of drupal past mentioned that all service classes are being loaded into memory at compile time anyway, so iterating with reflection on these classes to discover class-level `Hook` attributes has a negligible affect performance. @nicxvan also ran performance tests iterating reflection through each of a class's methods, and the hit on performance was not significant even with the number of methods in the class in 100,000s. Without an additional performance concern, it seems like a good idea to go forward.

Haven't completely found out why, but the DrupalKernel changes lead to the addition of Symfony\Component\DependencyInjection\Compiler\ResolveEnvPlaceholdersPass, and that makes it necessary to re-resolve the `%container.modules%` parameter to ModuleHandler in the test as well.

diff --git a/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
index 30cd8fe3b60..23decc3ba8f 100644
--- a/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
+++ b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5PluginManagerTest.php
@@ -167,7 +167,9 @@ private function mockModuleInVfs(string $module_name, string $yaml, array $addit
     // The exception to the above elegance: re-resolve the '%app_root%' param.
     // @see \Symfony\Component\DependencyInjection\Compiler\ResolveParameterPlaceHoldersPass
     // @see \Drupal\Core\DrupalKernel::guessApplicationRoot()
-    $container->getDefinition('module_handler')->setArgument(0, '%app.root%');
+    $container->getDefinition('module_handler')
+      ->setArgument(0, '%app.root%')
+      ->setArgument(1, '%container.modules%');

     // To discover per-test case config schema YAML files, work around the
     // static file cache in \Drupal\Core\Extension\ExtensionDiscovery. There is

benjifisher → credited godotislate → .

These are probably tricky enough that we should do them in multiple issues, we could maybe re-title this one to just cron and open a new meta for the others?

Cloned to 📌 [meta] Replace lazy service proxy generation with service closures Active as the meta and retitled here.

godotislate → created an issue.

Also a general question - is this the last lazy class in core?

Checking now, looks like there are still several others. Not sure on the history of this issue for why only cron is being converted. Tagging for an IS update. Either this issue should be a meta, with individual issues addressing each proxy class, or maybe an update on why only cron is being converted.

LGTM

No, this is just a very new rule, was not enforced before.

Oh, interesting. I could've sworn the 80-character limit for comments was an old rule, but good to know.

Added comments to the MR with a few thoughts.

Is it weird that the bot is picking up PHPCS issues that the build is not?

smustgrave → credited godotislate → .

FYI for future reference, I think using __METHOD__ can save having to type ConfirmFormBase::getFormName(), etc.

Commented on MR with suggestion to meet CS line length issue flagged by NR bot.

NR bot hopefully appeased.

Well if it's meant to be permanent in Drupal shouldn't that be announced? Also if permanent SpacelessBCExtension is the BC part needed?

There probably should be a CR for what the support policy will be for spaceless, but there needs to be agreement/direction on what that should be. There are questions about whether this should be done at all, or Drupal should emit a its own deprecation message, or if it's long-term support.

I don't think that SpacelessBCExtension specifically needs a CR, since it's internal implementation details that's not API.

OK to add it but should be also deprecated

Twig is emitting its own deprecation message for spaceless that it will be removed in Twig 4. AFAICT, there's no calendar for the Twig 4 release, so while we can add a deprecation message here for how long Drupal will support it, I think that won't be known until we know what eversion of Drupal will use/require Twig 4. Assuming that we want to support spaceless at all.

Can we get a CR for new extension?

I don't think it needs one? It's just there to override Twig's spaceless filter to suppress its deprecation message. I've added final and @internal to the class be sure.

Updated per MR review.

Addressed MR feedback

lgtm

Is this not considered an API change?

Yes, but this really is in "Needs review" only to solicit opinions/feedback, and if there is more clarity and enthusiasm for a path forward, we can document the agreed approach and API change.

Will doing the deprecation alleviate concerns about renaming? Adding the container parameters aliases the old class name to the new class name and provides BC, though the added typehints would be a breaking change for any implementing subclass methods.

Looks like all the latest feedback has been addressed. Back to RTBC,

Searched and found no other usages of file_test.results or file_test.return in the code base. LGTM.

I was able to the test-only job to fail by changing the test line from drupalGet to submitForm, so that a reset button press is actually performed.

@tregonia With the MR diff applied, I was not able to reproduce the your reported issue with anonymous session entries in the database table, using the Standard profile install steps detailed in the IS. Are you using any custom or contrib modules that touch forms or views? One other thing to check is to inspect the Reset button in the browser and check whether the name="op" attribute is on the button element. If it's not, then something is removing the attribute that core form API adds.

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

instead of TraitSafeClassLoader how about MissingClassDetectionClassLoader

Funny enough this is what I also came up with.

We'd be able to put the classloader registration/unregistration behind a PHP version check in Drupal 11.4-ish though.

I wonder if the classloader, or a similar one, should be kept in place, just to set a flag whenever there is a missing class/interface/trait. The missing class/interface detection right now is based on '/(Class|Interface) .* not found$/' regexp match, which is brittle and will need to be expanded to include traits. Instead of that, the classloader could set a flag for any detected missing dependency, and when catching the exception, use that flag instead of the regexp match. Actually, I think that can be done now, with the removal of trait aliasing being the only necessary change?

Nice!

@berdir can you elaborate on what you mean in 🐛 ContentTranslationManager::isEnabled() can load/create the same config entity many times Active comment 7/10?

b) Clone entities that we're about to change in \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval so that we alter things that others might depend on in their logic.

Which entities should be cloned?

There are test failures which have to be unrelated and known recurring failures. I think this is RTBC otherwise.

LGTM.
Removing Needs Tests and Needs subsystem maintainer review since it looks like both were already done.

One note about this solution is that since one multi-value insert query is replaced by multiple insert queries, the performance gain from creating multiple at once is lost. I don't have an alternate solution of how to address this. I thought about doing a select query to get the last count(items) right after the insert query, but to do that, need to be sure that there were no other insertions. Which would mean using a lock and makes things much more complicated.

The code basically relies on the indexing of the array to always match. Instead of making that assumption, we'll just use whatever is passed in and ensure the indexing matches up.

While it's certainly safer to ensure the keys align, looking at the code in QueueService::commitAdding(), I can't see how the keys would be anything other than sequential 0-indexed, since the array_chunk() call does not preserve keys. Nevertheless, if it indeed addresses an issue, then the keys need to be preserved in all implementations of QueueInterface::createItemMultiple(), so I've made the respective changes to MemoryQueue and QueueBase.

I looked at how to write some tests for this, but I'm not sure it's possible to change the autoincrement setting of the database in tests, and otherwise mocking the database service is a huge lift. Leaving at Needs tests for now.

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

CR added: https://www.drupal.org/node/3512253 → (@mikelutz provided helpful docblock that could be copied).

Note to update the CR if it's decided that method names need changing.

Are service providers considered internal, and not API? Service providers aren't mentioned specifically in the BC policy → . The closest thing is that compiler passes are considered internal, and it certainly seems like service providers should not be considered API. (It's also really hard for me to imagine how contrib/custom modules would be extending core service providers.)

But if that is not generally understood, then with the service provider classes being moved/split, there probably should be deprecation messages to that effect. Classes that are moved can use the new container parameters from https://www.drupal.org/node/3509577 → to deprecate the old class name.

MR looks good. I have 1 minor comment, but it's personal code style preference, and I don't feel that strongly about it.

@erwangel I am unable to reproduce your issue with the provided config. Can you provide reproduction steps starting with a core install with standard or minimal profile?

If not that, perhaps a screen recording of what actions you are performing and the error.

I wonder if there's value in generalizing an approach to prevent certain batch sets from being added twice. My thinking is to add a static array property to BatchBuilder. Something like:

class BatchBuilder {

  protected static array $ids = [];

  public static function hasId(string $id): bool {
    return !empty(static::$ids[$id]);
  }

  public static function setId(string $id): void {
    static::$ids[$id] = TRUE;
  }

  ...
}

Then in node_access_rebuild(), it'd be something like:

if ($batch_mode && !BatchBuilder::hasId('node_access_rebuild')) {
   ...
   batch_set($batch_builder->toArray());
   BatchBuilder::setId('node_access_rebuild');
   ...

And this could be applied similarly for other uses.

@erwangel can you provide the keys and values for all the allowed values? Translated ones as well, if they were set.

We could add a static in the function itself to ensure it's not added from there more than once. I guess the difference is someone could add it without calling this function.

By "add it," do you mean the static variable? If the variable is regular static and not drupal_static, then it can't be accessed outside the function. If you mean adding the batch set, if someone is doing that without using this function, then the id property might not get added, either. So the static variable might be a cleaner way to go?

Alternatively, if a property in array is being used, it could be on the $batch array itself and not within the set, to save from having to loop:

// Only recalculate if the site is using a node_access module.
if (\Drupal::moduleHandler()->hasImplementations('node_grants')) {
  if ($batch_mode) {
    // Ensure that the batch set is only registered once.
    $batch = &batch_get();
    if (!empty($batch['has_node_access_rebuild'])) {
      return;
    }
    $batch['has_node_access_rebuild'] = TRUE;
    ...

@arusahijpal Can you re-run the failing jobs in the pipeline? They should be unrelated, but it's better to make sure the tests are all green.

lgtm

Setting/detecting an arbitrary array property on the batch set feels strange, but I don't see a better way to detect whether a certain batch set is already registered, though.

One small nit on the MR, but otherwise looks like only tests are outstanding.

It was suggested as a possible performance enhancement on 📌 Investigate possibilities to parse attributes without reflection Active #16. I have not done any metrics on it.

godotislate → created an issue.

Rebased and removed the unused variable flagged by the nr-bot.

Note that in MR 11141, the test does not address Layout Builder permissions and field configuration specifically, but does generic config entity dependency order in a similar fashion. I had difficulty recreating the previous test MR that was against 10.4.x to 11.x, and a kernel test is more efficient than a functional test.

Relatedly, the value of the render array #access property and the return value of the render array #access_callback property can be booleans or AccessInterface objects. It's probably worth considering whether those should be enforced to be access objects as well.

There is already a pull request with a fix.

They said several years back that traits and interfaces were much more difficult to deal with:

To support the same for interfaces/traits, the basic idea would be to have a separate opcode like COMMIT_CLASS, which separates the addition of the class to the hashtable from the binding procedure, in which case all binding errors could become exceptions. This part I would assume to be relatively straightforward. The issue is that the inheritance logic also modifies the class in-place, this means that it's not possible to run inheritance on a class entry twice (e.g. after defining a necessary interface that previously threw). Solving that part wouldn't be so simple.

Though they clearly starting throwing exceptions for missing interfaces as well sometime between then and now.

We started using this in the hook ordering issue here: #3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter

I've reverted the HookCollectorPass changes here then, so only TwigExtensionPass is changed per the suggestion.

Addressed comment.

Created 📌 Use container builder to track and reuse reflection classes in compiler passes Active to reuse reflection classes in the container builder per #16. Changes were small and straightforward, so MR is ready.

godotislate → created an issue.

With 📌 Add a classloader that can handle class moves Active now in, rebased and used that to remove I18nQueryTrait.php from content_translation.
Also added docblocks to the new classloader methods, and in doing so, missingClass didn't seem to fit, so renamed to missingTrait.

This should be ready again.

@godotislate is was tested badly! Great spot. This works with the APCu optimised classloader great.

Nice!

new \Drupal\Core\Foooo();

 'Drupal\Core\Fooo':
      class: 'Drupal\Core\StringTranslation\TranslatableMarkup'

The class names don't match here, 4 o's instead of 3. Is this exactly how it was tested?

lgtm

Funny enough, the order of operations between && and = was also the problem behind 🐛 Exposed filter values ignored when using batch Needs work .

A couple comments on the MR. I think there might be missing test coverage for a private default image set on field config instead of field storage config.

I see the change record, so removing tag. I have a couple more suggestions after looking again.

Rebased to resolve the merge conflict.

Unrelated, but came up in testing against Drupal 11.1.3. The file validators need to be updated per https://www.drupal.org/node/3363700 → . So this section in Drupal\media_bulk_upload\Form\MediaBulkUploadForm::buildForm():

    $validators = array(
      'file_validate_extensions' => [implode(' ', $this->allowed_extensions)],
      'file_validate_size' => [Bytes::toNumber($this->maxFileSizeForm)],
    );

needs to be

    $validators = array(
      'FileExtension' => ['extensions' => implode(' ', $this->allowed_extensions)],
      'FileSizeLimit' => ['fileLimit' => Bytes::toNumber($this->maxFileSizeForm)],
    );

Also in media_bulk_upload_dropzonejs.module on latest 3.0.x (3ab898b1), this section in media_bulk_upload_dropzonejs_form_media_bulk_upload_form_alter() needs to be changed as well:

if (isset($form['file_upload']['#upload_validators']['file_validate_extensions'][0])) {
  $form['file_upload']['#extensions'] = $form['file_upload']['#upload_validators']['file_validate_extensions'][0];
}

to

if (isset($form['file_upload']['#upload_validators']['FileExtension']['extensions'])) {
  $form['file_upload']['#extensions'] = $form['file_upload']['#upload_validators']['FileExtension']['extensions'];
}

However, the contents of media_bulk_upload_dropzonejs_form_media_bulk_upload_form_alter() are moved to the new MediaBulkUploadDropzoneJsForm class in this MR, so the change would need be done in that class for this MR. The change to the validators for D11 compatibility seems to be out of scope for this issue, so if it's done in another issue, either the work there or work here will need to be rebased, depending on whichever is merged first.

One comment since it looks like the change_url is required in the deprecation message.

Though one thing about I18nQueryTrait is that it was decided to keep the original trait intact and not a wrapper for the new trait so that the new trait could have type declarations. See #55 of 🐛 Move content_translation I18nQueryTrait to migrate module Active .

#3502882: Add a classloader that can handle class moves might fix the i18nQueryTrait issue

Yes, agreed. Should this issue be postponed on that one? Then maybe the InlineBlock conversion shouldn't be done here so it isn't a blocker for 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work ?

Rebased MR and did the following:

• Moved the class_exists check back below retrieving from file cache, per #26
• Converted InlineBlock to attributes, per #29
• Moved the StubTrait and class loader to Drupal\Component\Discovery, per #12

Also, the I18nQueryTrait deprecation warning came up because of the class_exists() checks. At first I wrongly moved the new Trait location out of the Plugin/migrate/source folder, which had no effect because the original deprecated trait was the one causing the issue. Instead of this, I set a custom error handler right before the class_exists() check to suppress any deprecation warnings, which I hope is kosher. Still, this is another reason to move non-plugin files out of the plugin discovery directories, and one thing to consider for 📌 Parse attributes before annotations Active is whether to also use an error handler that suppresses deprecation warnings when instantiating Reflection in parseClass().

All tests pass now, so back to NR.

Made the change per #17 to editor_load() and deprecated after conversation on Slack. Added a CR.

FYI: New hooks hook_entity_duplicate() and hook_ENTITY_TYPE_duplicate() have been committed to Drupal core and will be available in Drupal 11.2 → . A universal approach that works for all the cloning modules might be possible using those hooks, when sites are running 11.2+.

Updated CR and IS because the committed hooks did were hook_entity_duplicate()and hook_ENTITY_TYPE_duplicate(), without the _alter suffix.

I believe the editor config entity type is statically cached

Actually, looks like no, it's not. Only Role is opted in?

If we make the editor config entity type statically cacheable (if it's not already), would we need the dedicated static cache here at all?
I believe the editor config entity type is statically cached in via entity storage, but I think the weird issue here is that there are formats that could possibly not use an editor, so we'd have to track format IDs that an attempt to editors for but don't exist.

Even without statically caching the editor entities here, I think we'd still want to statically cache the attachments/settings.

Refactored this a little to use Editor::loadMultiple() with a set of IDs passed in, instead of editor_load(). This way there is only one query to load the editor entities, but also leverages the entity cache and does not call listAll().

One possible issue that came to mind is that it is possible that editor entity CRUD actions during a request, so the statically cached copies of attachments and editors in the EditorManager would be stale. This doesn't seem to be all that likely, except possibly in tests, but this doesn't seem to affect existing tests since they all pass. It's possible to bring in memory cache-like functionality, but that increases complexity.

Not sure if there are content entity types that have string ids, or config entity types with int ids.

Every content entity type extending ContentEntityBase will have an integer ID field (see baseFieldDefinitions), so only content entity type classes that do not extend that base class could implement string IDs. This seems like an edge case, but it might not be good to be restrictive either (hypothetical example would be a content entity type that only uses UUIDs as IDs.)

I don't think it matters whether config entities have numeric IDs or not, since they would all get treated as strings. And if someone has created a config entity type with a serial ID expecting a large number of entities, that seems a like an architectural issue wrt scale.