Handle NULL source field value

@krystalcode Would you be able to provide the steps necessary to reproduce this?

This issue makes me a bit uneasy because redirect_source is a required value - i.e. the entity is invalid without it - i.e. broken. So maybe the exception here is correct behaviour and the calling code that causes this error needs to be more aware. @krystalcode how to run into this issue?

Also if we fix this then we should also fix:

• \Drupal\redirect\Entity\Redirect::getSourceUrl()
• \Drupal\redirect\Entity\Redirect::getRedirect()
• \Drupal\redirect\Entity\Redirect::getRedirectUrl()

What is also fun is that

• \Drupal\redirect\Entity\Redirect::getSourcePathWithQuery
• \Drupal\redirect\Entity\Redirect::getRedirectOptions
• \Drupal\redirect\Entity\Redirect::getRedirectOption
• \Drupal\redirect\Entity\Redirect::getHash
• \Drupal\redirect\Entity\Redirect::getStatusCode

All work on an entity that's invalid...

Maybe given that discovery the correct thing to do here is to make it work without triggering an error.

Okay I've added fixes for all the getters and tests for them on an empty entity. Will leave to maintainers to decide whether this approach is correct. I get the feeling it is. It lets callers be more flexible.

@alexpott

A couple of comments in the MR. In the original patch I used the isEmpty() method because it is the API method that takes into account how the field type defines "empty". For this field type, here is the definition of empty.

https://git.drupalcode.org/project/redirect/-/blob/8.x-1.x/src/Plugin/Fi...

Real world applications are messy and you can end up with unexpected values due to bugs. Based on this definition, a value like ['path' => '', 'query' => []] should be ignored and the field should be considered as empty. Not sure how other getters handle these, but as an API method I think it's cleaner for the caller to return [] in such cases, and the full value can be retrieved by get('redirect_source') if needed. It would be a bit inconsistent if the field item list would tell you that the field is empty while the getter would give you a value.

Also, the null-safe operator (?->) was introduced in PHP 8.0. this module does not define its compatibility neither with Drupal core nor with PHP in composer.json, but it is declared as compatible with Drupal 9.2+ in its info file which is compatible with PHP 7.3 and 7.4. This makes the module installable in settings where this would cause fatal error.

If the module has reached the point where maintainers only wan to support PHP 8.0+ or Drupal 10.0+, then it should be declared accordingly. But I don't think that should be done just for this issue, I'm personally a bit more conservative and trying to not break other people's websites.

@rajesh.vishwakarma @alexpott

I do not recall the exact steps to reproduce this. But more or less I believe it was something like the following. I was working on extending the form validation at RedirectForm::validateForm(). The parent method from ContentEntityForm would build the entity, and if the source had an invalid value submitted (when creating new redirects), then I would call $entity->getSource() and come across this error.

This was while developing and it may be the case that you wouldn't come across this in normal circumstances. However, I do think that it would still be good to handle this gracefully.

I'm not sure if there is a more general convention in Drupal core/community regarding the level at which a required field should be enforced. You first create the entity at the storage level i.e. $storage->create([ ... some field values ... ]). The bundle (if the entity is bundleable) is enforced there. I believe I have seen some entity reference fields required at that level for some entities.

Other than that, in general, entities are still created and usable with missing required fields. I think that's correct because you may need to do calculations to set the values before you reach the point of being able to set the values for all fields. Then the caller should ensure that - when needed - entity validation is called before saving the entity and that would catch these errors.

If module maintainers want to enforce this value to never be empty from the beginning of the entity's lifecycle, I think this should be happening at the $storage->create() level - then the getter may be left to throw an error. Otherwise the getter should handle this gracefully.

@krystalcode great point about the Drupal 9 support - agreed with @Berdir that we're going to drop that in 📌 Drop Drupal 9 and PHP 7 support Active - dropping Drupal 9 support doesn't break their sites FWIW.

Even though the minimum version of Redirect is now Drupal 10 I've made the change to isEmpty() as per @krystalcode's comment.

Merged.