[meta] Deprecate __get/__set() on ContentEntityBase

Created on 21 May 2022, about 2 years ago
Updated 10 October 2023, 8 months ago

Problem/Motivation

(Content) Entity API is hard to understand, one aspect of that is there there are multiple ways of doing things and a lot of magic.

This is an attempt to deprecate __get()/__set() on ContentEntityBase *only* (magic methods on field item and field item list classes is a different topic).

There are two different aspects to this:
a) Accessing fields, this can simply be deprecated 1:1 by using get()/set() instead, which only supports fields.
b) Non-field properties like ->original and other cruft, mostly leftovers. This is tougher, for official things like ->original we should add a method, for random stuff we could consider some kind of temporary storage, there is an issue for that that I would need to dig out.

( πŸ“Œ Fix ContentEntityBase::__get() to not return by reference Needs work , was only about the by-reference return, this is about deprecating it entirely.)

Steps to reproduce

Proposed resolution

Remaining tasks

Prerequisites/Blockers

* πŸ“Œ Add a method to access the original property Needs work / #1480696: Move $entity->original to a separate hook argument β†’
* #2896474: Provide an API to temporarily associate data with an entity β†’

User interface changes

API changes

This will have a huge impact on mostly custom code, accessing fields through magic methods is very common.

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Postponed

Version

11.0 πŸ”₯

Component
EntityΒ  β†’

Last updated about 10 hours ago

Created by

πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    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.

  • πŸ‡¬πŸ‡§United Kingdom joachim

    > 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

  • πŸ‡©πŸ‡ͺGermany geek-merlin Freiburg, Germany

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

    Coming from a place πŸ“Œ Allow field types to control how properties are mapped to and from storage Needs review 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!

  • πŸ‡©πŸ‡ͺGermany geek-merlin Freiburg, Germany
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    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().

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    Heavy +1 on this initiative.

    I also found this issue after I reported this issue on 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

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • Status changed to Postponed 8 months ago
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

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

  • last update 8 months ago
    Patch Failed to Apply
  • last update 8 months ago
    Patch Failed to Apply
Production build 0.69.0 2024