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?