Add static cache to Migration FieldableEntity::getFields

This seems fine. I was thinking about whether it would be a good idea to use a more robust caching system here, but I don't see any advantage, and a few disadvantages. Using drupal_static will get fresh data each process, so we don't have to worry about clearing it if we re-run migrations after changing the data. The amount of data should be reasonable, it scales based on the number of entity types and bundles which should be limited, so I don't see any memory issues arising. most importantly we aren't caching data in memory that might scale based on the number of rows or anything like that. I also don't think that this optimization requires any additional explicit tests. The fact that the entire migration suite of tests run and pass prove that it works.

Adding tests for D10.

This is a good find.

However, drupal_static() is on its way out, so we shouldn't introduce new usages: #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() .

This should either use a class property, or the memory cache backend (see discussion in #3047289: Standardize how we implement in-memory caches).

As per comment #8, the patch has been re-rolled.

We strongly discourage static variables in classes - this should use a class property or the memory cache backend.

Static variables have been replaced by class-protected variables.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
@@ -20,6 +20,14 @@
+   * Memory cache for cache items.
+   *
+   * An associative array of Cache values and its CID.

I think we can improve on this. How about just one summary line, Stores the field configuration data for each entity field.

There is also a blank line before and after the line that begins $cid = . For me it would be a bit easier to read is the blank line fore it was removed.

Should we do the same for D6 in the d6 node source plugin?

Here I have made changes as per comment #13, please review.

I think we should do the same for D6.

Should we do the same for D6 in the d6 node source plugin?

I'm pretty green on D6 migration, but it looks like the D6 node source already does static caching for field definitions. In the d6 node source, getFieldInfo does the same job as FieldableEntity::getFields(). And getFieldInfo already uses a static cache using its $fieldInfo property. Here's the property definition:

  /**
   * Cached field and field instance definitions.
   *
   * @var array
   */
  protected $fieldInfo;

So I don't think there's any additional work to do on this issue. I think we should be happy with #14. If we were dying for more work we could change the variable name $cache to $fieldInfo to be consistent with what's already in the D6 node source, but I don't think that's a requirement. I'm happy enough to RTBC. If a migrate maintainer disagrees, though, I know the community would be happy to make a quick update. :)

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
@@ -19,6 +19,13 @@
+  /**
+   * Stores the field configuration data for each entity field.
+   *
+   * @var array[]
+   */
+  protected $cache = [];

Given this is an abstract class and designed to be extended this name feels way too generic for me.

Let's call it $fieldNameCache instead. So it's a bit more descriptive about what it is.

It's tempting to not add this to $this but use a static variable in the function.

Also this is overriding \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::$cache with a completely different type of thing so very interesting things could happen here.

Made changes as per comment #16, please review.

Per @catch in #11:

We strongly discourage static variables in classes - this should use a class property or the memory cache backend.

That makes me uncomfortable using a static variable here. Can we go back to using a protected property but use the new name?

And I would be inclined to use $fieldInfo as the name since that would match the D6 node source as described in #15. We might as well use a name that's already in use for doing this exact thing somewhere else.

Attached patch uses patch from 14 with changed protected property name to match that of the Drupal 6 node source plugin here

Two interdiffs for comparison.

Thanks, @jasonawant!

It looks like you missed updating instances of $this->cache to $this->fieldInfo.

That I did! Thanks for calling that out. Here's an updated patch and interdiffs.

That looks great, @jasonawant. And thanks for the thorough interdiffs as well as the link in #19. I'm going to throw this back to RTBC. Thanks!

Changing title to state the change being made here.

I am doing triage on the core RTBC queue.

The issue summary is clear. The proposed resolution is not correct, this is not using a static cache. It is a simple change so I have updated the issue summary. I read the comments and found that all points have been addressed.

Above I'd suggested class property or using memory cache, given this is a trait, using MemoryCache is less straightforward, so class property seems fine.

Committed/pushed to 11.x, thanks!