[meta] Deprecate __get/__set() on ContentEntityBase

Added reroll of patch #19 as it's not getting applied anymore, will work on it later for fixing tests.

I have mixed feelings about this.

Being able to write $entity->field_name->value is really nice DX. It's my preferred way to work with entity fields by far.

On the other hand, having both $entity->field_name and $entity->get('field_name') as valid syntax means that it's a pain to search your code for uses of a field (when refactoring, checking dependencies, etc etc). Having one syntax only would make that much easier.

But I think if we were to remove one way of accessing field values, I'd rather remove the get() and keep the magic.

Would this also include a deprecation of the magic __get method on FieldItemList.

I don't have a preferred approach, but i do agree here, that this would simplify (and greatly improve DX) if we could remove a lot of the ways of accessing field data, and go to one standardized approach.

This would indeed be a disruptive change for contrib and custom code, but it can be cleanly done through deprecating.

Is there any chance this could come with a phpcbf plugin or something to replace such usages? This way of doing things has been around for the better part of a decade and there are going to be a great many unhappy people if we have to replace every usage manually.

I agree with @joachim in #23 here. It's nice to define bundle classes with @property annotations so the IDE and phpstan know the types of these magic properties. Not sure ::get() can achieve the same.

> Being able to write $entity->field_name->value is really nice DX. It's my preferred way to work with entity fields by far.

Counterpoint: I've previously encountered a PHP bug where the magic __get() on a particular class won't get invoked recursively. So if you have a chain of calls that includes this:

- $entity->foo
-- __get() calls get()
--- your get() implementation access another property on the same entity, or on another entity of the same type, $entity->bar
---- magic __get() is NOT invoked, because you're already inside one

+10 on this, and hooray when it's done.

Coming from a place where $entity->$field returned an array instead of a fieldItemList. No wonder given the polymorphy of this __get() beast (it returns fields, properties, ad-hoc values).

In the era where static type checking and IDE support saves us from >90%, doing this is a blessing.

PS: #27: Huuuuu, i did not know that!

Some additional context/thoughts around my +1'ing the deprecation of magic methods.

I agree with @joachim in #23 here. It's nice to define bundle classes with @property annotations so the IDE and phpstan know the types of these magic properties. Not sure ::get() can achieve the same.

I am a huge fan of bundle classes as well and think it's one of the most underrated improvements to Drupal in the D9 lifecycle. That said, I'm not sure a property annotation for the class is better than adding methods to retrieve the specific field items in question - with the added benefit that since this is by definition business logic, you can return either a field item list, a collection of loaded entities, a single entity, or even a specific value depending on the known characteristics (e.g., cardinality and backing data type) of the field. Instead of the boilerplate you'd still have to do, say, on an entity reference field with cardinality 1: $entity->field_name->first()->entity (itself a kinda troublesome magic method getter) and still not having a typehint on the kind of entity you're getting. Much nicer to do $entityBundleClass->getTheThingWithKnownType().

This is an instance of the PHP issue with recursive magic methods: #3213247: Loading an order item's order when the order needs refreshing crashes with Argument 1 passed to Drupal\commerce_tax\Plugin\Commerce\TaxType\TaxTypeBase::buildCustomerProfile() must implement interface Drupal\commerce_order\Entity\OrderInterface that I mentioned in #27.

Heavy +1 on this initiative.

I also found this issue after I reported this issue on a Drupal module that was probably the first, maybe still the only, module that introduced- and proved that "not every entity is content or config". It was a hell of a job delivering the first versions of the module in the Drupal 8 era and it only turned out recently that I have still forgotten about magic methods... :facepalm:

My gut says they are rather a bug in Drupal core because every code that depends on them relies on interfaces (contracts) that do not enforce the existence of magic methods on an implementation.

#23, what I would add is that all that said I like and sleep better when a static code analyzer understands my code and covers my back. So I would rather keep get() and set().

+1 on #30

Maybe the status should be also postponed (until the blockers are resolved) as per #22.

Wacky idea: what if we WROTE PHP code for field access on cache rebuild, the same way Twig files are compiled?

We could then write PHP setters and getters for all the fields an entity type has.

Interesting idea, then definitions could be described via protobufs for example

This gets a -1 from me, at least in its current form. We use __get() in all of our projects as the primary method for retrieving entity fields because of how brief it is, so this would be a very time consuming change for us to have to deal with.

One alternative that seems interesting to me is to simply make __get() work only with fields. We could alias __get() to get(), but one distinction that immediately comes to mind between them is that the former doesn't throw an exception when the field doesn't exist. This is behavior that we rely on fairly heavily since it provides a simple, safe way to access fields when you don't care if they exist:

// Currently, this is possible:
if ($entity->field_fizz_buzz?->value) {
  // do something
}

// What it would become:
if ($entity instanceof FieldableEntityInterface && $entity->hasField('field_fizz_buzz') && $entity->get('field_fizz_buzz')->value) {
  // which is needlessly more verbose...
}

// This is slightly better:
if ($entity instanceof FieldableEntityInterface && $entity->get('field_fizz_buzz')->value) {
  // but it's *not* safe if the field doesn't exist; you'll get an exception
}

This seems rather nit-picky, but when multiplied 1,000 times across a project it becomes very time consuming.

I understand the benefit that type inference would bring, but this would be a huge cost for existing projects.

Re: the conversion burden this might cause, this seems to be the kind of thing that can be aided by a rector rule?

I figure it would be impossible for rector to catch even a majority of the work that would need to be done because it will largely rely on the developer's diligence to use or notate proper type information; I assume you'd need to be able to establish the ContentEntityBase type via static analysis. Consider even the following scenario with accurate type information:

// This would be missed entirely because we can't leap from EntityInterface to ContentEntityBase.
// I'm also assuming runtime information would be unavailable (e.g., entity type ID).
function hook_entity_presave(EntityInterface $entity) {
  if ($entity->field_some_field?->value) {
    // do stuff
  }
}

I assume these scenarios would likely be fine:

function hook_entity_presave(EntityInterface $entity) {
  // Node -> ContentEntityBase
  // Can we make the leap from NodeInterface?
  if ($entity instanceof Node && $entity->field_some_field?->value) {
    // do stuff
  }
}

// Entity type-specific hook with generic interface.
function hook_node_presave(EntityInterface $entity) {
  /** @var \Drupal\node\Entity\Node $entity */
  if ($entity->field_some_field?->value) {
    // do stuff
  }
}

// Entity type-specific hook with entity type-specific interface.
// Node -> ContentEntityBase
// Can we make the leap from NodeInterface?
function hook_node_presave(Node $entity) {
  if ($entity->field_some_field?->value) {
    // do stuff
  }
}

I assume the following would also be fine, but I'm less certain. Will custom entity interfaces be an issue (e.g., class Custom extends ContentEntityBase implements CustomInterface)? Would we lose out on detecting CustomInterface, have false positives, ...?

// With a little bit extra work, we can intuit more specific types for some hooks:
// hook_ENTITY_TYPE_presave -> Custom -> ContentEntityBase
function hook_custom_presave(EntityInterface $entity) {
  if ($entity->field_some_field?->value) {
    // do stuff
  }
}

This change almost begs for the magic methods to be part of some interface that EntityBase implements. This would require a minimum of two major releases for BC concerns as far as I know.

These assumptions could be completely wrong, and if they are, then I will happily reassess.

we need to deal with original and also other stuff that's thrown on entities at the moment. And there's no movement at the moment on those issues.

This isn't universally true. See #2839195: Add a method to access the original property which is getting closer and closer and just needs a little more love.

#40: would this work?

// $entity->$fieldName->$property becomes
array_shift(array_column($entity->get($fieldName)->getValue(), $property));

You get a null from missing field, missing property and empty field.

#2839195: Add a method to access the original property is in which unblocks this.