Problem/Motivation

WorkspacePublisher::publish() wraps the entity save loop in a try / catch (\Exception $e) block that rolls back the open transaction, logs the exception via Error::logException(), and rethrows. PHP \Error subclasses (\TypeError, \AssertionError, \Error from assert(), etc.) are not subclasses of \Exception, so the catch block is bypassed when one is thrown from inside the closure (for example from a hook implementation or downstream service call invoked during $entity->save()).

Steps to reproduce

  1. Track two entities in a workspace.
  2. Trigger a \TypeError (or any \Error subclass) from hook_entity_presave() on the second of the two entities.
  3. Call $workspace->publish().
  4. Observe that the first entity's workspace revision was published to Live and no error was logged on the workspaces channel.

Proposed resolution

Widen the catch in WorkspacePublisher::publish() from \Exception to \Throwable. Error::logException() already accepts \Throwable, so no other change is required in the catch body.

Remaining tasks

Review.

User interface changes

Nope.

Introduced terminology

N/A

API changes

Nope.

Data model changes

Nope.

Release notes snippet

N/A

Issue fork drupal-3590350

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
plach’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Looks good and works well here, thanks!

godotislate’s picture

Status: Reviewed & tested by the community » Needs review

One question on the MR.

smustgrave’s picture

Status: Needs review » Needs work

Not a huge fan of the hooks inside kernel tests but it is valid and probably worth doing. Moving to NW for that.

amateescu’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

Feedback appears to be addressed

  • godotislate committed 24b03581 on main
    fix: #3590350 WorkspacePublisher doesn't roll back when a PHP Error is...

  • godotislate committed f239996b on 11.x
    fix: #3590350 WorkspacePublisher doesn't roll back when a PHP Error is...
godotislate’s picture

Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 24b0358 to main and f239996 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

amateescu’s picture

Version: 11.x-dev » 10.4.x-dev
Status: Fixed » Reviewed & tested by the community

As a bug fix, this should go into 11.4.x as well. I checked and it can be cherry-picked cleanly :)