- π¦πΊ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!
- πΊπΈ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()
. - π¬π§United Kingdom joachim
This is an instance of the PHP issue with recursive magic methods: π 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 Active that I mentioned in #27.
- ππΊ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
- Status changed to Postponed
8 months ago 7:52am 10 October 2023 - ππΊ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