[meta] Deprecate __get/__set() on ContentEntityBase

Hopefully this won't break the testbot. Trying with deprecation messages for those magic methods to see what happens. whitelisted a few very widely used properties for now.

Tested with a bunch of entity kernel tests to also see the change this has and had to fix a bunch of cases in the storage classes.

Obviously we'll need to split that out in various issues, passRaw for example is an interesting one, not sure yet how to handle that. user cancel already seems to basically a BC layer, very weird.

coding standard fixes.

Workaround around broken HEAD to get some results.

The last submitted patch, 4: content-entity-magic-3281720-4.patch, failed testing. View results →

That went about as well as expected :)

There's a bunch in the installer, improving that, a bunch of changes in node and user modules, RevisionLogEntityTrait and storage as well as ignoring two more generic cases (referringItem from entity reference formatters and sessionId from drupalLogin()), results in no more deprecations in NodeCreationTest for example.

That's what I wanted to write, anyway, but then coding standards complained about the validation property that I tried to define... 83 | WARNING | Property name "$_skipProtectedUserFieldConstraint" should not be prefixed with an underscore to indicate visibility. meh.

The last submitted patch, 6: content-entity-magic-3281720-6.patch, failed testing. View results →

More exclusions for properties that need dedicated issues, fixing a bunch more generic cases like terms and migrations.

Meh.

The last submitted patch, 9: 3281720-9.patch, failed testing. View results →

That's not as much of a difference in number of "failing" tests as I hoped, but counting the lines, it's about 2000 fewer deprecation messages, just not all of them for most of these tests.

Definitely shows that we need a temporary state API for all the things like _referingItem, view, book, and all those other common properties. And some should become a proper API.

I'm whitelisting all those generic ones for now, they will need separate issues, not sure about the order yet. maybe introduce that as an api with some examples and a generic BC layer for magic methods, then switch them over in dedicated issues? means we will need to wait with @trigger_error until all of these are switched over.

workspace module was kind enough to add a @todo to issue for that that I was to lazy to look up: #2896474: Provide an API to temporarily associate data with an entity →

Another bunch of various replacements, focusing on actual code and common cases. This should result in a considerable amount of tests no longer failing.

Side note: As expected, there's a good chunk of fun stuff being discovered here. For example dozens of entity tests using 'language' => 'en' but the field name is langcode, so all those lines never did anything.

The last submitted patch, 11: 3281720-11.patch, failed testing. View results →

Another ~2k deprecation lines less than before.

Most common remaining ones:

grep "content entities" output.html | cut -d: -f2 | sort | uniq -c | sort -nr
    312  Accessing fields (name) as properties on content entities is deprecated in drupal
     88  Accessing fields (status) as properties on content entities is deprecated in drupal
     66  Setting a field value as a property (langcode) on content entities is deprecated in drupal
     61  Accessing fields (bundle) as properties on content entities is deprecated in drupal
     58  Setting arbitrary properties (body) on content entities is deprecated in drupal
     56  Accessing fields (thumbnail) as properties on content entities is deprecated in drupal
     54  Accessing fields (moderation_state) as properties on content entities is deprecated in drupal
     50  Checking for an undefined property (in_preview) with isset() on content entities is deprecated in drupal
     50  Accessing fields (workflow) as properties on content entities is deprecated in drupal
     46  Accessing fields (pass) as properties on content entities is deprecated in drupal
     44  Accessing fields (user_picture) as properties on content entities is deprecated in drupal
     44  Accessing fields (moderation_state) on content entities is deprecated in drupal
     40  Checking for an undefined property (bundle) with isset() on content entities is deprecated in drupal
     37  Accessing fields (parent) as properties on content entities is deprecated in drupal
     37  Accessing fields (content_entity_revision_id) on content entities is deprecated in drupal

And also a long tail of random/unique test cases for all kinds of fields, in total 840 unique messages still. Migrate is a common issue, apparently the row source is often/usually passed through unchanged and everything is unconditionally set, on new entities anyway, updates check for it being a field.

This is starting to get very big, several big search and replaces in here for bundle, thumbnail, status, moderation_state, status and {$field_name} field access patterns. Also, changing migration entity destination to only set known fields, similar to update.

And a bunch more generic excludes for properties.

Oops.

The last submitted patch, 15: 3281720-15.patch, failed testing. View results →

I'm sorry, but there's quite a bit of very weird stuff in content_moderation. Tons of non-get() based access to fields, super-weird mocking of fields using stdclass, that langcode removal trickery in \Drupal\Tests\content_moderation\Kernel\ContentModerationStateTest::testNonLangcodeEntityTypeModeration, also there's just no way that isFirstTimeModeration() is doing what it's supposed to.

Some cleanup there, but also still some failing tests.

Also fixing jsonapi and rest test base class which should fix hundreds of tests.

The last submitted patch, 17: 3281720-17.patch, failed testing. View results →

Various other fixes. the twig thing is a bit tricky, unsure about that one. About time to put this aside and focus on property issues like origin and temporary data.

The last submitted patch, 19: 3281720-19.patch, failed testing. View results →

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

Thanks for the reroll. It's probably not too useful to chase HEAD with this issue. I kind of stopped here because we need to focus on the child issues first and probably split this up further, it's getting way too big.

First, I'd work on 📌 Add a method to access the original property Needs work , that is a huge DX improvement on its own if we can finally get that done.

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 📌 Allow field types to control how properties are mapped to and from storage Needs work 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: 🐛 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.

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

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->hasField('field_fizz_buzz') && $entity->field_fizz_buzz->value) {
  // which is needlessly more verbose...
}

// This is slightly better:
if ($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.

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.

I'm aware that the impact of this is massive. This isn't going to happen any time soon. We're many layers of issues and new APIs away from achieving that. See related issues, 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.

There are two aspects on your example.

The first is the problem that get() throws an exception on unknown fields. That used to make sense, but since ?-> exists, that indeed seems awkward, and I've been thinking about removing that exception for a while now. It would be great if you could start an issue to propose that. The good thing is that our API doesn't enforce the return type, so we can maybe change the behavior. The only problem I see is someone is relying on the exception and is catching that, then it would result in a php warning or something instead. like this:

<?php
try {
echo $entity->get('field')->value;
}
catch (\Exception $e) {
}

Lets discuss that in that issue. One hack that we've done before on behavior changes is to allow to control it with Settings, so you can opt in to the new behavior and will be the default in D12.

The second part of your comparison is exactly *why* I want to deprecate those methods. Your example implicitly assumes that you have a content entity with magic __get(), if not, that logic would result in PHP warnings. And no, $entity?->field?->value doesn't protect against that, only against $entity being null. Your code that's working with entities is going to be in a function, be it a hook or an API, and you can already type hint there. If your method is only typed to EntityInterface then you'd already get that warning if you receive a non-content entity.

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 📌 Add a method to access the original property Needs work which is getting closer and closer and just needs a little more love.

#40: would this work?

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

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

I created 📌 Remove exception when accessing a non-existing field with ContentEntityInterface::get() Active . 📌 Add a method to access the original property Needs work is now RTBC.

We'll need #2896474: Provide an API to temporarily associate data with an entity → then, some more stuff, like an issue to improve twig/content entity integration, and then reroll this huge patch.