Remove exception when accessing a non-existing field with ContentEntityInterface::get()

Proof of Concept MR

Turns out that core does on fact have one example that relies on the exception being thrown.

But good news is that the functional test was random.

I actually created 📌 Provide FieldableEntityInterface get() functionality that returns NULL instead of throwing exception Active before finding this issue, so closed that one as a dupe. There I proposed adding a second parameter to get() or a new get-like method that returns NULL instead of throwing an exception, to account for anything relying on the exception being thrown. So either

/**
   * Gets a field item list.
   *
   * @param string $field_name
   *   The name of the field to get; e.g., 'title' or 'name'.
   *
   * @param bool $throw_exception
   *   Whether to throw an exception if field name is invalid or return NULL.
   *
   * @return \Drupal\Core\Field\FieldItemListInterface|null
   *   The field item list, containing the field items.
   *
   * @throws \InvalidArgumentException
   *   If an invalid field name is given.
   */
  public function get($field_name, bool $throw_exception = TRUE);

or

/**
   * Gets a field item list safely.
   *
   * @param string $field_name
   *   The name of the field to get; e.g., 'title' or 'name'.
   *
   * @return \Drupal\Core\Field\FieldItemListInterface|null
   *   The field item list, containing the field items, or NULL if invalid field name.
   */
  public function safeGet(string $field_name): ?FieldItemListInterface {
    try {
      return $this->get($field_name);
    }
    catch (\InvalidArgumentException) {
      return NULL;
    }
  }

A parameter is the solution to the return behavior BC break, but it comes with its own BC problems of adding arguments to an existing method on an interface. Likely has a smaller impact though, I hope ContentEntityBase::get() wasn't customized much. We could introduce it with a default of TRUE and then change that.

In slack fetch() was also proposed, but I'm not really sure that "Usually fetch calls considered to be returning something or null" is such a common standard, certainly haven't heard about that but might be common in some frameworks? Fetch to me sounds like it actively fetches something from somewhere, while get just returns the available data.

Changing the signature to get() to have two arguments or adding a "safe" method to FieldableEntityInterface would both introduce a BC break. I think it'd make more sense to do the former, if for no other reason that it doesn't require an additional change to the Twig allow list. (Though I imagine most Twig templates use the magic getter instead of get).

I think I'd also prefer an argument on the existing method, that also matches ContainerInterface::get(). The problem is that even if we don't add it to the interface for BC, it still breaks subclasses that don't have it (you can add optional parameters that parent classes/interfaces don't have, but you can't not have optional parameters that parent classes/interfaces have).

The only BC safe way would be some func_get_args() trickery.

> (Though I imagine most Twig templates use the magic getter instead of get).

FWIW, I created this issue from the discussion in 📌 [meta] Deprecate __get/__set() on ContentEntityBase Needs work , which currently requires to change twig templates. I want to create a twig issue to improve it's handling of content entities anyway, so the twig templates remain as they are, but node.foo.value actually translates to $node->get('foo')->value by default. Either with throw_exception = FALSE OR a hasField() check. And on the field level too, because node.reference_field.entity is really unpredictable right now in twig (if there is no entity, it falls back to getEntity(), which returns the host node. even I got bitten by that several times).

The only BC safe way would be some func_get_args() trickery.

I think this might make sense, along with a deprecation triggered. Is there also a way to annotate in the interface that the method parameters will change?

OK, went ahead with the second argument to get() as mentioned in #7-10. Followed the steps for adding a method parameter from How to deprecate → . Since the method signature will need to be updated by subclasses anyway in D12, I think the $field_name parameter should be typehinted to string and the return value should be typehinted to FieldItemListInterface, but I'm not sure what the policy is on how to declare a return type addition.

CR https://www.drupal.org/node/3522223 →
Follow up for D12: 📌 [PP-1] Add typehints and $throw_exception parameter to FieldableEntityInterface::get() and default to FALSE Postponed

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Re-rolled.

I guess we accidentally cross-pushed the reroll, so I re-added a change in .deprecation-ignore.txt to put the new rules under the right header.

Since it's not likely this makes 11.2.0, bumped the deprecation version to 11.3.0.

Reviewed.

Ah, you added this to the change record:

> The default behavior will change in Drupal 12.0.0, where $throw_exception will have a default value of FALSE, so calling get() with an invalid field name and without a second argument value will return NULL. In addition, the method signature in the interface will change to include the $throw_exception argument, as well as a string typehint to the $field_name parameter.

I think we don't need to do that and keep it like that. That's a bigger change that's more likely to affect code. I think if we do that, we'd need to make it a D13 deprecation, but this seems to complicate things quite a bit.

As for 11.2 vs 11.3, I think the BC impact of this is trivial, nothing in core subclasses this method and it should be very rare in contrib/custom too. Without the D12 change, risk to callers is also basically zero as nothing changes unless explicitly requested, and it's quite useful, I plan to RTBC that once the review comment is addressed and then we can maybe still get it into 11.2 before beta.

I think we don't need to do that and keep it like that. That's a bigger change that's more likely to affect code. I think if we do that, we'd need to make it a D13 deprecation, but this seems to complicate things quite a bit.

My understanding was that there are a couple instances in core (and unknown how many in contrib/custom) where an exception is expected/handled on an invalid field passed to get(), so throwing an exception should continue to be the default.

But once we get to D12 (or 13), it'd be more convenient not to have to do get($field_name, FALSE) to get a NULL return, when it seems likely that the NULL return is the desired behavior most of the time. Doing this would mean that very no uses of get() need to change now, but in the major change, only usages of get() that have explicit exception handling would need to be changed to get($field_name, TRUE), and other existing usages of get() without explicit exception handling would not need to change.

I'll make the MR changes if it's still preferred to keep the behavior the same through the next major.

But once we get to D12 (or 13), it'd be more convenient not to have to do get($field_name, FALSE) to get a NULL return, when it seems likely that the NULL return is the desired behavior most of the time. Doing this would mean that very no uses of get() need to change now, but in the major change, only usages of get() that have explicit exception handling would need to be changed to get($field_name, TRUE), and other existing usages of get() without explicit exception handling would not need to change.

That's all true. switching the parameter would avoid the ", FALSE" in the future and it's very likely a now or never thing, because once we have it, switching the default is going to be much harder.

But the deprecation for it is pretty awkward and the possible impact for it is also bigger. ignoring our own deprecation messages is something we very much try to avoid (it's a slippery slope. Recovering and cleaning up deprecations in core from around 8.0 like entity.manager took _years_) and it makes it extremely hard for possibly affected code paths to be notified about it.

For me, it's either default TRUE with D12 deprecation or default FALSE with D13 deprecation. default TRUE means we can properly document and expose it earlier and personally, I'm willing to pay for that with the ", FALSE". But if others disagree on that then I won't insist. I do think we'll need to extend the deprecation to D13 though for the other version.

For me, it's either default TRUE with D12 deprecation

Made the changes to this in the MR and CR. I think I prefer the other way, but I'd like to see this get in, and even doing , FALSE is better than hasField() checks.

There's probably also still a bit more time to do a deprecation to flip the default value before D13 if wanted as well, and I can update this MR again if others prefer it the other way.

Had an idea, so opened a second MR (MR 12123) to do the deprecation with the expectation that the default will change from TRUE to FALSE. I moved the deprecation warning so that it only triggers when an exception is thrown and the second parameter is not set, and updated relevant tests to expect the deprecation message.

For clarity:

MR 10406: $throw_exception default is TRUE with the expectation it will continue to default to TRUE

MR 12123: $throw_exception default is TRUE with the expectation it will be changed to default to FALSE in Drupal 13.

Two things came up resolving tests that need answers though:

1. ContentEntityBase::set() calls get(). We probably either need to do the same with set() and add an optional $throws_exception so it can be passed to get(), or we have set() call get() explicitly with $throws_exception as TRUE, so exceptions will always be thrown for invalid fields
2. get() is documented in FieldableEntityInterface to throw an InvalidArgumentException on invalid field names, but in ContentEntityBase::getTranslatedField, which is called by get(), it's also possible to throw an exception when "The entity object refers to a removed translation". My thought then is that not all \InvalidArgumentExceptions should be suppressed, but only ones with message "Field $name is unknown", but this seems brittle

My thought then is that not all \InvalidArgumentExceptions should be suppressed, but only ones with message "Field $name is unknown", but this seems brittle

Thinking about this gave me an idea that it could be convenient to do exception handling using enums instead of codes or messages, so I created ✨ Introduce Exception type that allows Enums be to used to set code Active . Not a blocker for this, but I think it could be useful for similar cases in the future.

Changed the parameter name from $throw_exception to $exception_on_invalid, because that's what PluginManagerBase::getDefinition() uses, and makes sense to me to be consistent.