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.

Can 1 MR be closed or hidden. Or summary updated what the difference of the 2 are if one option needs to be picked over the other

Thanks.

They both have nearly the same commits. The difference is that !12123 adds some deprecation handling to tests and throws an exception. The commits that differ are the commits after "Changes from MR feedback". It seems likely that the best course of action would be close !10406.

Or summary updated what the difference of the 2 are if one option needs to be picked over the other

IS has been updated with the decision to make between the two MRs.

See #20-22 for the original discussion as well.

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

I think set() shouldn't silently ignore what you pass to it, so I'd suggest we explicitly tell it to throw an exception.

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

We could introduce a new exception for unknown fields then we can deal with that specifically. That should be backwards compatible if it extends the existing exception.

godotislate → changed the visibility of the branch 11.x to hidden.

Updated both MRs per #38.

Another review. FWIW, you've convinced me to default to not throwing an exception in D13 and going with that MR.

MR update per review comments.

godotislate → changed the visibility of the branch 3490787-remove-exception-when to hidden.

Going on a limb and say all feedback appears to be addressed on this one.

Code seems clean and don't have any comments. The CR really helped me with the review because it showed where it's going. Also the before/after examples excellent.

All threads are closed and pipeline green so believe good to go.

Of course as soon as I did that the pipeline showed unmergable.

Rebased, will move to NR once tests pass.

Tests passed.

Record response!

Nearly committed this but realised I have one question.

What would be the problem with removing the exception entirely, without adding the parameter? Code that wants to throw an exception if a field doesn't exist can check ::hasField() first and throw an exception then - e.g. EntityAdapter could do this.

> What would be the problem with removing the exception entirely, without adding the parameter?

Not 100% sure I understand the question, at first I thought it's related to the BC layer, there we have to deal with existing code that uses the exception instead of a hasField() check, like the core/modules/field/src/Entity/FieldStorageConfig.php change in core.

But I realized that I think you mean in the "final form" in D13 and whether we need the ability to ever throw an exception. That's a very good point. Initially we had this inverted, so you had to pass an argument to not get an exception, similar to Container::get() or PluginManager::getDefinition(). Then it was necessary.

I think we do need it now, so that code can benefit from it in D11.3-D13 without triggering the exception/deprecation message, stuff like:

      if ($body = $comment->get('comment_body', FALSE)?->value) {

But we wouldn't need it in D13. So maybe instead of planning to define the argument explicitly in D13, we just plan to remove it again then? We could invert the BC layer to then trigger a deprecation message if you pass in FALSE. And we could also change it now so that only FALSE is valid and not accept TRUE as we'll remove that ability?

at first I thought it's related to the BC layer, there we have to deal with existing code that uses the exception instead of a hasField() check, like the core/modules/field/src/Entity/FieldStorageConfig.php change in core.

That was to some extent deliberately vague. I wondered about somehow deprecating the exception itself, so that if it's thrown and caught, it would trigger the deprecation notice indicating that code should use hasField(), but I can't remember an equivalent problem and didn't think it all the way through yet.

If that's not viable then yes something like

But we wouldn't need it in D13. So maybe instead of planning to define the argument explicitly in D13, we just plan to remove it again then? We could invert the BC layer to then trigger a deprecation message if you pass in FALSE. And we could also change it now so that only FALSE is valid and not accept TRUE as we'll remove that ability?

should work.

Note that set() calls get(), and in previous comments we decided that set() shouldn't fail silently on unknown fields.

We can do the hasField() check within set() and throw the exception from there, though.

I wondered about somehow deprecating the exception itself, so that if it's thrown and caught, it would trigger the deprecation notice indicating that code should use hasField(), but I can't remember an equivalent problem and didn't think it all the way through yet.

Lack of precedent aside, I think this is an interesting idea. Though if we're still deprecating for Drupal 13, then we'd still have to do exception handling until then, which is a bit longer than I would hope.

Not sure we'd have to deprecate for Drupal 13 in this case because in general you should not be using exceptions for code control, hasField() already exists and is the actual API to use. The exception is supposed to tell you 'this field doesn't exist, are you sure you used the right field name'. It looks like the goal of this issue is move towards more of a 'garbage in garbage out' where typos fail silently, but fields that may or may not exist are more convenient to deal with. But code that is really trying to make sure it doesn't load a non-existing field because it knows it might not be there should already be using ::hasField() IMO.

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.

Rebased

OK, put up MR 12845 with the approach to just deprecate throwing the exception. Will update the CR and follow up issue to match if this turns out to be the preferred way to go.

It looks like the goal of this issue is move towards more of a 'garbage in garbage out' where typos fail silently, but fields that may or may not exist are more convenient to deal with.

I've been on a project or two where fields are removed from entities, but devs miss removing code like $entity->get({REMOVED FIELD NAME}). Without adequate test coverage, it goes to prod and results in a WSOD.

This could also affect Twig templates and if and when the entity magic getter is removed and replaced with get() instead.

It looks like the goal of this issue is move towards more of a 'garbage in garbage out' where typos fail silently, but fields that may or may not exist are more convenient to deal with.

I've been on a project or two where fields are removed from entities, but devs miss removing code like $entity->get({REMOVED FIELD NAME}). Without adequate test coverage, it goes to prod and results in a WSOD.

Well, that is then in many cases traded into php warnings (accessing property on null) or fatal errors ($entity->get('field')->anyMethodCall()). This is isn't meant to address that.

For me, this is specifically about enabling the usage of the nullable operator as mentioned in the issue summary. It would be neat if we could specifically detect that, but obviously that's not possible.

Not sure we'd have to deprecate for Drupal 13 in this case because in general you should not be using exceptions for code control, hasField() already exists and is the actual API to use

Your call, but I think we shouldn't underestimate the impact this will have on custom code.

This change will result in code like this:

      try {
        $entity->get('field_name')->getValue();
      }
      catch (\InvalidArgumentException) {
      }

Should you do that? No. But I'm fairly certain there are hundreds of cases of custom code out there doing exactly that. IDE's like PhpStorm typically even tell you that you should add that try/catch because it's documented to throw that. With the exception removed, this will be a fatal error.

I'm confused why MR !12845 doesn't need the change in \Drupal\field\Entity\FieldStorageConfig::getOptionsProvider. That should trigger the deprecation now, no?

I'm not very fond of that approach without a way to explicitly disable the exception now. It makes it impossible to actually do if ($entity->get('field_name')?->value) until we are on D12/13 and remove the exception.

My reason for pushing this is proceeding with 📌 [meta] Deprecate __get/__set() on ContentEntityBase Needs work and this came up as an argument why people prefer $entity->$field_name over $entity->get($field_name), because that does return NULL and allows this.

Well, that is then in many cases traded into php warnings (accessing property on null) or fatal errors ($entity->get('field')->anyMethodCall()).

Yeah, my thought was incomplete. But basically I'd find it more convenient guarding against that type of error with a nullsafe operator.

I'm confused why MR !12845 doesn't need the change in \Drupal\field\Entity\FieldStorageConfig::getOptionsProvider. That should trigger the deprecation now, no?

The exception still needs to be handled until we stop throwing it (in a follow up presumably), so that's why there's no change in that class. And apparently there's no test code that would throw the exception there, so no deprecation message emitted.

I'm not very fond of that approach without a way to explicitly disable the exception now. It makes it impossible to actually do if ($entity->get('field_name')?->value) until we are on D12/13 and remove the exception.

I agree. I can look into your suggestion in #50 instead, maybe next week.

And apparently there's no test code that would throw the exception there, so no deprecation message emitted.

I'm suspect there's something more sinister going on.

I found that code because just removing it did result in a fatal error. See the very first pipeline of the original MR: https://git.drupalcode.org/issue/drupal-3490787/-/jobs/3542502. Drupal\Tests\options\FunctionalJavascript\OptionsFieldUI failed there with a fatal error.

That _should_ be a deprecation now.

But this is most likely triggered by an ajax call through the browser. And because our deprecation handling relies on HTTP headers, deprecations in ajax requests are silently dropped. I've seen that before.

With the exception removed, this will be a fatal error.

If we remove the exception itself, then yes, but if we remove the exception throwing and not the exception itself, then it will only be a fatal error if the exception otherwise might have been thrown. And I think that might be fine for a Drupal 12 change because that code can be rewritten to use ::hasField(), then the actual exception can be deprecated for removal in Drupal 13.

And I think that might be fine for a Drupal 12 change because that code can be rewritten to use ::hasField(), then the actual exception can be deprecated for removal in Drupal 13.

Maybe I'm misunderstanding, but it's throwing \InvalidArgumentException, so there's no deprecation or removal needed for the exception itself.

We can change the code in \Drupal\field\Entity\FieldStorageConfig::getOptionsProvider to use hasField instead of exception handling though.

Maybe I'm misunderstanding, but it's throwing \InvalidArgumentException, so there's no deprecation or removal needed for the exception itself.

No I just hadn't checked which exception it was actually throwing properly before typing...

That there is no specific exception for this makes it IMHO pretty hard to detect and identify affected code, I'd expect most is custom code that won't see deprecation messages, not without the issue that logs them.

That's my reason to make this a D13 deprecation, and as a result of that, have a way to explicitly bypass the exception to use the nullsafe operator. If you think D12 is fine, then I won't push back against that further and we can possibly skip the argument, if we think that maintaining that magic parameter isn't worth it.

No I think that's fair enough, I think the main question then is how to handle adding and then removing the parameter again vs. making it permanent?

godotislate → changed the visibility of the branch 3531287-loosen-sebastiandiff-constraint to hidden.