Problem

  public function preSave(EntityStorageInterface $storage) {
    // Check if this is an entity bundle.
    if ($this->getEntityType()->getBundleOf()) {
      // Throw an exception if the bundle ID is longer than 32 characters.
      if (Unicode::strlen($this->id()) > EntityTypeInterface::BUNDLE_MAX_LENGTH) {
        throw new ConfigEntityIdLengthException("Attempt to create a bundle with an ID longer than " . EntityTypeInterface::BUNDLE_MAX_LENGTH . " characters: $this->id().");
      }
    }
  }

The documentation in the interface doesn't say that this method can throw an exception.

I would argue that it should be allowed to throw any type of exception and not just ConfigEntityIdLengthException, as entity classes may have other things that they must check.

(For instance, with Flag module we want to throw a \LogicException.)

Novice task

Add @throws documentation to EntityInterface::preSave() that says it throws \Exception when there is a problem that should prevent saving the entity.

See https://www.drupal.org/node/1354#throws or other examples of @throws tags in Core for a model of how to do this.

CommentFileSizeAuthor
#12 2662152-12.patch572 bytesrafaolf
#9 2662152-9.patch634 bytesrafaolf
#8 2662152-7.patch634 bytesrafaolf
#6 2662152-6.patch634 bytesrafaolf
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Version: 8.1.x-dev » 8.0.x-dev
Issue tags: -needs backport to 8.0.x

Good call. Probably we should document on the interface that classes may throw (generic) exceptions (using @throws), and that this stops the save process. At least... I assume that is what happens? Hard to tell. I am not actually sure where/how this is called.

Fixing version... we are currently marking issues 8.0.x if they are OK to go into either 8.0 or 8.1, rather than marking them 8.1 and tagging with backport. This will eventually need to change but that is what we are doing now.

joachim’s picture

joachim CreditAttribution: joachim commented

> that this stops the save process. At least... I assume that is what happens?

Looking at how the code core I posted throws an exception for a badly-formed bundle ID, I'd say it stops the save process.

AFAICT nothing catches these. Which makes sense, as they're pretty much showstopper problems with the entity.

The chain looks like:

- Entity::save(), which is what UI or custom code will call
- EntityStorageBase::save()
- EntityStorageBase::doPreSave()
- Entity::preSave($this);

The presave hook is invoked immediately after.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue summary: View changes
Issue tags: +Novice

Ah, great, thanks for the detective work!

So I think this would be a reasonable Novice project... We should:

Add @throws documentation to EntityInterface::preSave() that says it throws \Exception when there is a problem that should prevent saving the entity.

See https://www.drupal.org/node/1354#throws or other examples of @throws tags in Core for a model of how to do this.

Updating summary with this task.

rafaolf’s picture

rafaolf CreditAttribution: rafaolf at CI&T commented
Assigned: Unassigned » rafaolf

Going to look into this.

rafaolf’s picture

rafaolf CreditAttribution: rafaolf at CI&T commented
Assigned: rafaolf » Unassigned
Status: Active » Needs review
FileSize
2662152-6.patch634 bytes

Status: Needs review » Needs work

The last submitted patch, 6: 2662152-6.patch, failed testing.

rafaolf’s picture

rafaolf CreditAttribution: rafaolf at CI&T commented
FileSize
2662152-7.patch634 bytes
rafaolf’s picture

rafaolf CreditAttribution: rafaolf at CI&T commented
Status: Needs work » Needs review
FileSize
2662152-9.patch634 bytes
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks for the patch! It needs some work before it is ready to be added to the Drupal Core source code though:

  1. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -309,6 +309,9 @@ public function delete();
+   * ¶

    Extra space at the end of this line. You should be able to set up your programming text editor or IDE to either highlight or remove end-of-line spaces, which will help you avoid this problem in your next patches. 

  2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -309,6 +309,9 @@ public function delete();
+   * @throws Drupal\Core\Config\Entity\Exception\ConfigEntityIdLengthException
+   *   In case a configuration entity ID is too long an exception is thrown

    This is too specific. EntityInterface::preSave() can throw any type of exception, for any reason, and it indicates that the entity should not be saved -- see issue summary under "Novice task".

    So rather than using this specific type of exception, please use the generic \Exception class, and rather than this specific description, use a more generic description -- again see issue summary.

    Also the end of the description line(s) should be a .

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Clarification on previous comment, item 2: Do something like this:

@throws \Exception
   (reason here)
rafaolf’s picture

rafaolf CreditAttribution: rafaolf at CI&T commented
Status: Needs work » Needs review
FileSize
2662152-12.patch572 bytes

@jhodgdon

Thanks for the tips here.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Thanks! Yes, that is more what we are looking for.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

SqlContentEntityStorage catches any exceptions thrown during saving, then re-throws an EntityStorageException re-using the information from the original exception that was thrown (which could be anything).

That's documented at https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

That made me think we didn't need the docs here, since preSave isn't really a part of the public API - it'll only be used by entity storage classes, but it make it clearer to entity storage classes that they need to handle exceptions thrown here.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed e1d2e51 on 8.1.x 
    Issue #2662152 by rafaolf: Entity::preSave() throws a exception, but...

  • catch committed 4d4be42 on 8.0.x 
    Issue #2662152 by rafaolf: Entity::preSave() throws a exception, but...
joachim’s picture

joachim CreditAttribution: joachim commented

> That made me think we didn't need the docs here, since preSave isn't really a part of the public API - it'll only be used by entity storage classes

We're planning on using preSave() in Flag module, to check for application logic (eg 'does this flagging entity's declared flaggable entity make sense with the flag type?'). Is there something we should be using instead?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Another note is that every method that throws exceptions should document that fact with @throws. It seems like it is part of the entity API that preSave() is *supposed* to throw exceptions to block entity save, or at least quite a few of our core entities are using it to do that. Therefore, it seemed right to document it on the interface, especially as the individual entity preSave() methods just use @inheritdoc to inherit that documentation.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

pwaterz’s picture

pwaterz CreditAttribution: pwaterz commented

Side question, what's is the purpose of the 32 character limit? It would be nice to see a comment of why that code exists.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Definitely a side question. :) The 32-character limit has to do with limiting the size of names of config items, which are constrained to be a total of 255 characters (or something like that) due to having to fit in a varchar database field. With entities, the config names get really long for things like field settings etc, because they include the type of field, name of field, type of entity (like node), subtype (bundle), and probably some other things I didn't think of. So there are limits to the length of each piece that could go into such a name.

pwaterz’s picture

pwaterz CreditAttribution: pwaterz commented

Gotcha, thanks for the info. It would be nice not to have this limit, but that's a discussion for another day. Cheers!