Provide an API to temporarily associate data with an entity

I think we could do a property_exists() in the magic __set() method and then throw deprecated warning. Checking in the __get() may not catch it b/c.

->original has its own issue to be deprecated in favor of setters/getters.

I think we should add the API, then open a follow-up to add a deprecation warning just to see what happens and then as part of that open specific issues to convert usages in core to that.

I'm also open to discussing to deprecate $content_entity->$field_name in favor of supporting only get()/set(). I'm not sure yet about field properties.

See #1977266: Fix ContentEntityBase::__get() to not return by reference for a special case of __get() that we can likely not do anymore on its own but we might want to focus on the cases that require it to be by reference here first as they are probably the weird ones.

What about a seperate repository service or factory instead of adding additional responsibilities to EntityInterface. Could look like something like the following:

/** @var EntityTemporaryDataRepositoryInterface $temp_data */
$temp_data = \Drupal::service('entity.temporary_data_repository');
$temp_data->get($entity, 'foo');
$temp_data->set($entity, 'foo', 'bar');

/** @var EntityTemporaryDataRepositoryInterface $temp_data */
$temp_data = \Drupal::service('entity.temporary_data_factory')->getRepsitory($entity);
$temp_data->get('foo');
$temp_data->set('foo', 'bar');

I like:

/** @var EntityTemporaryDataRepositoryInterface $temp_data */
$temp_data = \Drupal::service('entity.temporary_data_factory')->getRepository($entity);
$temp_data->get('foo');
$temp_data->set('foo', 'bar');

a lot.

Especially as we could just potentially wrap it in a decorator when we need to access the additional properties:

$entity = new TemporaryMutableEntity($entity, \Drupal::service('entity.temporary_data_factory'));

However the disadvantage of keeping it separate from the entity itself is while it ensures that lazy builders could use "pack()" the entity back to array('entity_type', 'entity_id'), the problem is that when code actually relies on that there is no way anymore to see if the entity was modified.

e.g. what I used in a proof-of-concept while developing BigPipe was to serialize() the entity and serialize() a freshly loaded entity and compared those to see if it was modified in some way.

We could also allow to add a list of EntityModifiers - instead.

While that is still arbitrary, it is at least consistent.

e.g.

What about:

$modifier = new EntityModifier();
$modifier->set('_initialPublished', TRUE);
$entity->addModifier($modifier);

A get on an unused property would then ask if any modifier has the property and return it if it is there.

Though that does still not solve the problem of when an entity is reloaded, though e.g. the _initialPublished should not even persist for longer than the entity's lifetime.

---

So we have two things to take into account here:

• a) Ensure that properties survive a reload of the entity (for certain cases, others are relying on the exact object instance).
• b) Ensure that a diff of original entity vs. modified can still be retrieved somehow.

Still brainstorming here ...

I think the problem we have is that the entity has a fixed interface and we want / need to enhance that interface during the runtime. [e.g. for the a theoretical EntityWorkspaceStatusInterface, which has isInitialPublished() / setInitialPublished() methods].

Also some things must persist in entity cache, while others are just temporary to the object.

Which is kinda what I though of with my modifiers:

$entity = new DynamicInterfaceEntity($entity);
$entity->addDynamicInterface('EntityPublishedInterface', $entity_published_interface_provider);

with the big problem that an instanceof won't work unless it is a true dynamic child entity, but if that code was changed to check for:

$entity->hasDynamicInterface('EntityPublishedInterface') it would work for both.

Brainstorming end ;).

there's existing API for that purpose
- user_data service, associates and removes additions to user entity
- third-party-settings for config entities

and less known
- key value entity storage - #2208617: Add key value entity storage

In addition to #21, another way to do it is to define a computed field on the entity, which returns a NULL value. You can then set $entity->mycomputedfield->value and retrieve it later from the same entity object.

I'll drop in here and suggest that some variation of JSON storage for this could be a nice way to interact with this and avoid some of the weirdness in the user data model, which is truly prehistoric Drupal.

Coming from #3281720: [meta] Deprecate __get/__set() on ContentEntityBase I think there are kind of two things here.

One is a 1:1 replacement of the current magic __get()/__set() stuff. ->origin is now dealt with, but there are things like _referringItem, inPreview, view and various others. They are not expected to be persistent, but mostly on purpose only for the specific *object* as opposed to a given entity (identified by entity id/uuid).

Then there's this bit:
> Unfortunately this practice is completely unreliable; whenever static caches are cleared, or an unchanged entity is loaded from the database, a new entity will be instantiated and the temporary data is lost.

I think temporary kind of implies that it's... temporary? I think the persistent thing adds a fair bit of complexity that is somewhat unclear just how persistent it is? I'm tempted to say that if you want something persistent then do one of those custom service things so you control it? Possibly even like user.data which makes it actually persistent. Unsure what the exact use cases are for that.

Started a merge request with a basic implementation.

@bradjones1: I don't think this is related to *storage* at all, it about temporary, not-persisted data. The user.data service is something entirely different.

I think the persistent thing adds a fair bit of complexity that is somewhat unclear just how persistent it is?

The way I interpreted it, it seems the OP is talking about persistence throughout the request/response cycle. From time to time, you need to "persist" information between two points of the execution flow with no clean way to pass that around. In the dark days we would use a $GLOBAL variable for that, but, for one, that's not compatible with the request stack. A saner alternative could to be to attach attributes to the request itself, but that may force you to inject the current request or the request stack into a context that has nothing to do with those concepts, which is suboptimal as well.

Granted that the need to pass data around like this is itself not ideal and likely an indication that something is wrong in the subsystem relationships or the logic being implemented, I tried to address this use case for a few custom projects of mine through this code, which is close in spirit to what was proposed in #8. Something like that would support both the temporary entity data use case and the "persistent" one.

Personally, I'd like to drop entirely the habit of setting random data on entity objects.

Personally, I'd like to drop entirely the habit of setting random data on entity objects.

I'd agree with this. There are other options (e.g., the tempstore) that work for most of the use cases described.

Yes, I agree that storing information that should "persist", in some form or another typically should be done in a different way.

FWIW, tempstore is specifically not this use case either because that is used to store data across multiple requests.

That said, the simple use case for a single loaded entity that should explicitly _not_ persist across clones and static cache clears is IMHO somewhat valid, and if we want to deprecate magic get/set then we need at least an intermediate solution for this. Some things like isPrevie should be handled by the specific entity type as it's a feature they provide (using an interface + trait for example), but what about _referringItem, set during rendering an entity reference? There's the ->view property that's set when an entity is rendered within a view.

Storing that elsewhere requires some workarounds like storing it by object hash which can have weird edge cases (the documentation on that explicitly mentions that hashes may be reused), so I think storing that directly on the entities is a good fit.

@berdir

That said, the simple use case for a single loaded entity that should explicitly _not_ persist across clones and static cache clears is IMHO somewhat valid, and if we want to deprecate magic get/set then we need at least an intermediate solution for this.

I agree, but my concern is that, by adding an API to support that use case on the entity class itself, we are encouraging people to take advantage of this pattern even more, which is likely to result in abuse, since it's hard to tell a legitimate API usage (very few IMHO) from an invalid one (most of them IMHO 🙂).

Some things like isPrevie should be handled by the specific entity type as it's a feature they provide [...]

💯

what about _referringItem, set during rendering an entity reference?

Yes, this is a clear of example of an ephemeral/contextual property: a file can be referenced by multiple entities, so adding an explicit parent reference would make no sense and would not help here. IMO the right way to handle it would be something along these lines:

  /**
   * This is an alternative version of EntityReferenceFormatterBase::getEntitiesToView.
   * It is passed a Context object previously initialized with field item
   * contextual data (specifically the "_loaded" property). Additional contextual
   * information is added to it as the entities to be displayed are collected.
   * The Context class would implement \ArrayAccess and likely wrap a
   * \ WeakMap instance.
   */
  protected function prepareEntityDisplayContext(EntityReferenceFieldItemListInterface $items, string $langcode, Context $displayContext): array {
    $entities = [];

    foreach ($items as $delta => $item) {
      // Ignore items where no entity could be loaded in prepareView().
      if ($displayContext[$item]->get('loaded')) {
        $entity = $item->entity;

        // Set the entity in the correct language for display.
        if ($entity instanceof TranslatableInterface) {
          $entity = \Drupal::service('entity.repository')->getTranslationFromContext($entity, $langcode);
        }

        $access = $this->checkAccess($entity);
        // Add the access result's cacheability, ::view() needs it.
        $displayContext[$item]->set('access_cacheability', CacheableMetadata::createFromObject($access));
        if ($access->isAllowed()) {
          // Add the referring item, in case the formatter needs it.
          $displayContext[$entity]->set('referring_item', $items[$delta]);
          $entities[$delta] = $entity;
        }
      }
    }

    return $entities;
  }
  

The $displayContext would be passed around as needed and discarded once the render array is returned. No need for "persistence" at all in this particular case, I think, as all the consuming code is in the formatter classes extending EntityReferenceFormatterBase (at least in core).

There's the ->view property that's set when an entity is rendered within a view.

Another contextual property, I assume something like this would be viable in both the the occurrences I found:

  public function preRenderByRelationship(array $result, string $relationship): void {
    $view_builder = \Drupal::entityTypeManager()->getViewBuilder($this->entityType->id());

    foreach ($result as $row) {
      if ($entity = $this->getEntity($row, $relationship)) {
        // Remove this: $entity->view = $this->view;
        $this->build[$entity->id()] = $view_builder->view($entity, $this->view->rowPlugin->options['view_mode'], $this->getLangcodeByRelationship($row, $relationship));
        $this->build[$entity->id()]['#view_executable'] = $this->view;
      }
    }
  }

Storing that elsewhere requires some workarounds like storing it by object hash which can have weird edge cases (the documentation on that explicitly mentions that hashes may be reused) [...]

I believe using a WeakMap would solve this: the entity contextual data would no longer be available as soon as the original entity object were destructed.

@fabianx:

So we have two things to take into account here:
a) Ensure that properties survive a reload of the entity (for certain cases, others are relying on the exact object instance).
b) Ensure that a diff of original entity vs. modified can still be retrieved somehow.

IMO a) could be solved by having a contextual data storage relying on \SplObjectStorage or \WeakMap. Depending on the use case, one could instantiate an EphemeralContext class (relying on \Weakmap) or a PersistentContext one (relying on \SplObjectStorage).

I think we have (at least) one issue dealing with b) (#2862574: Add ability to track an entity object's dirty fields (and see if it has changed)), but, if we go with a separate contextual data storage, we don't have to worry about that here :)
Actually, a lazy builder could serialize the entity contextual data, which should be far cheaper than serializing the whole entity.

Maybe introducing a distinction between mutable and immutable entity objects, like we have for config, would help there (and would probably be desirable for other reasons), but that's definitely not a small undertaking :)

I think the problem we have is that the entity has a fixed interface and we want / need to enhance that interface during the runtime.

IMO this is exactly where the distinction between legitimate entity data addition and contextual data lies: (content) entity data should live in fields (stored or computed). When additional fields are defined that the base interface does not know about/support, you can always rely on decorators/wrappers to access them and benefit from static analysis and IDE autocompletion. The Content Translation module follows this pattern with its ContentTranslationMetadataWrapperInterface.

OTOH ephemeral data without a proper definition (including accessor methods on an interface), currently stored as a random property, only makes sense as contextual data and should not live on the entity IMO.

> I agree, but my concern is that, by adding an API to support that use case on the entity class itself, we are encouraging people to take advantage of this pattern even more, which is likely to result in abuse, since it's hard to tell a legitimate API usage (very few IMHO) from an invalid one (most of them IMHO 🙂).

Fair.

> The $displayContext would be passed around as needed and discarded once the render array is returned.

That's the tricky part. There's a surprising amount of places where ->_referringItem is available that would possibly need to be refactored to be able to receive that. FWIW, most places probably _do_ have access to the render array, similar to your view example, so that might be a feasible option.

That was my reason for going with the getOriginal() method in #2839195: Add a method to access the original property instead of #1480696: Move $entity->original to a separate hook argument as entities being saved are passed around so many hooks and methods.

I guess my concern is that if we have to solve every single instance of these current cases, deal with BC and so on then we won't be able to do something about #3281720: [meta] Deprecate __get/__set() on ContentEntityBase for many, many years. But it's a fair argument that we don't really gain much in the end if we replace it with a 1:1 API doing the same thing in a different color.

@berdir

That's the tricky part. There's a surprising amount of places where ->_referringItem is available that would possibly need to be refactored to be able to receive that. FWIW, most places probably _do_ have access to the render array, similar to your view example, so that might be a feasible option.

Right, we could definitely add the information to the render array instead of discarding it and deprecate accessing _referringItem. This should definitely be ok BC-wise.

That was my reason for going with the getOriginal() method

IMO this is completely fine: the original object is not random/contextual data, I cannot think of a more legitimate use of a property on an entity object :)
If we ever end up introducing the distinction between immutable and mutable entity object, then a ::getOriginal() method would only make sense on the latter (and return the former), similar to the ::isNew() one and friends, but for now I think that's a valid solution.

I guess my concern is that if we have to solve every single instance of these current cases, deal with BC and so on then we won't be able to do something about #3281720: [meta] Deprecate __get/__set() on ContentEntityBase for many, many years.

Maybe we could use magic methods to populate a globally available EphemeralContext instance and stop storing them on the entity object? This way, in the BC phase we can throw deprecation messages warning people that they should not use magic methods and that they should refactor their code to pass data around properly, but at the same time we can drop magic methods and leave the global EphemeralContext instance around for (at least) one more major release, so that people not having the option to refactor their code can access the global context directly instead of via $entity->__get()?

Pseudo-code ("tested" here):

abstract class ContentEntityBase {

  /* ... */

  public function &__get($name) {
    $context = \Drupal::service('entity.contextual_data.ephemeral');
    assert($context instanceof EphemeralContext);
    $data = $context[$this];
    assert(is_array($data));
    return $data[$name];
  }

  public function __set($name, $value) {
    $context = \Drupal::service('entity.contextual_data.ephemeral');
    assert($context instanceof EphemeralContext);
    $context[$this][$name] = $value;
  }

}

// ----------------------------

$node = Node::load(1);

// These throw 11.x -> 12.0 deprecation messages.
$node->foo = 'bar';
$bar = $node->foo;

// These throw 11.x -> 13.0 deprecation messages.
\Drupal::service('entity.contextual_data.ephemeral')[$node]['foo'] = 'bar';
$bar = \Drupal::service('entity.contextual_data.ephemeral')[$node]['foo'];

Using a service with a weakmap for this sounds good but also something like $entity->view I would assume was added 'just in case it will be useful' in about 2009 and we should deprecate it with no replacement. Would be good to ay least open issues for the spurious looking ones.

> but also something like $entity->view I would assume was added 'just in case it will be useful' in about 2009 and we should deprecate it with no replacement. Would be good to ay least open issues for the spurious looking ones.

We actually have a valid and for us important use case for this. We have a views display plugin that allows to control certain options on customizing displayed teasers, in this case a customized block display that is then used through paragraphs/block_field so editors can display a list of content and control how it looks without requiring 10 different view modes.

Same with _referringItem in combination with entity reference fields.

Both are tricky to use because it's on you to adjust cache keys of the rendered nodes, but it's possible.

That said, views uses it to provide theme suggestions for that (without supporting caching properly) and that we should deprecate: #2728419: Deprecate node/comment views-based theme suggestions and variables, but it's blocked on #3159050: Allow deprecating theme suggestions and that got stuck. So many issues...

You can see some of the more frequent cases that we have for this in the current __get()/__set() MR where I added an ignore list for them just so I could get a better overview of other remaining bits: https://git.drupalcode.org/project/drupal/-/merge_requests/10629/diffs#7...

So we have things like:
* pass_raw/passRaw, used by drupalCreateUser/drupalLogin on the user entity
* _skipProtectedUserFieldConstraint again for user entities, actually used by user module itself. Explicitly "internal" but also kind of an API.
* _restSubmittedFields, which is a workaround for not having an entity factory (to create an entity without default values), which is another chain of issues a decade+ old
* _initialPublished, which I think is workspaces
* in_preview/preview/preview_view_mode, as mentioned already
* book/rdf, which are removed from core by now
* _serviceId, is also replaced with a new solution now I think

A slightly different version of this but related issue that's a problem for config entities and the main reason we had to make them #[AllowDynamicProperties] is \Drupal\Core\Entity\EntityForm::copyFormValuesToEntity(). That just slaps all form state values into the entity using set() (content entities subclass this and check with hasField()). For config entities, set() behaves like __set() for content entities. BC around that will be fun. And there's some content entity deprecations triggered there on form submission that I still have to track down.

Opened an issue to remove _initialPublished from Workspaces: #3498115: Fix usage of temporary entity data in Workspaces

@berdir

Do you think the solution proposed in #34 would be acceptable/viable for the use cases you summarized? I guess we could skip deprecating the global contextual storage, at least until we are done cleaning up all those occurrences.