Problem/Motivation

The constructor for MigrateException accepts both $code and $previous parameters but doesn't pass along either to the parent call. This breaks exception chaining for any code that wraps a caught exception in a MigrateException.

An example of this can be found in the core MigrationLookup plugin where $previous would be lost: https://git.drupalcode.org/project/drupal/-/blob/7bd46e0ad93e1f554804d51....

Proposed resolution

Pass both parameters to the parent call.

Remaining tasks

Review.

CommentFileSizeAuthor
#9 3572219-nr-bot_lfbardlj.txt665 bytesneeds-review-queue-bot

Issue fork drupal-3572219

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:

About issue forks

Comments

jordandukart created an issue. See original summary.

jordandukart opened merge request !14685

jordandukart’s picture

jordandukart commented
Title: MigrateException does not pass $previous to parent constructor » MigrateException does not pass $previous or $code to parent constructor
Status: Active » Needs review

Added a unit test for this but may be overkill given what's changing. Also was unaware that these were captured as part of the baseline for PHPStan. Removed those ignores as CI flagged it when I opened the merge request on the issue fork.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Needs review » Needs work

@jordandukart:

Thanks for opening this issue, and starting the MR. It is always a good thing when we can remove a few lines from the PHPStan baseline! Also, this issue reinforces the lesson that PHPStan often gives good advice.

I made a few suggestions on the MR, and I see that you already accepted the first one. Back to NW for now; please change the status back to NR when you have considered the other suggestions.

In the issue summary, you reference a line in the migration_lookup plugin that is affected by this bug:

        throw new MigrateException(sprintf('A %s was thrown while processing this migration lookup', gettype($e)), $e->getCode(), $e);

I am not sure: is that usage covered by the test that you added? If not, can we add some additional test coverage?

jordandukart’s picture

jordandukart commented
Status: Needs work » Needs review

Had some discussion in Slack and added a Kernel test with a sample migration that shows this behavior. That Kernel test uncovered that the `processPipeline` call itself should also be updated as it wraps a caught MigrateException from lower in the chain but doesn't pass along properties from the caught exception.

It's a bit of an expansion of scope from the original issue being reported here but imo it's an additive change versus anything that'd break backwards compatibility. That being said, I could see why we may not want to change from the fallback value for $code even if it's present on the wrapped $exception. I added a commit adding the $previous exception as that should have no impact beyond preserving the chain. Open to discussion for or against in that regard if we wanted to set $code as well.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Needs review » Needs work

@jordandukart, can you reply to @heddn's question on the MR? I am setting the status to NW for that.

I also added #3084708: MigrateException should pass previous exception up the chain. as a related issue. I think we can close that issue as a duplicate, but I set it to NW for now.

jordandukart’s picture

jordandukart commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented

@benjifisher your thoughts on the latest changes?

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3572219-nr-bot_lfbardlj.txt665 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review

local blip

benjifisher changed the visibility of the branch drupal-3572219-3572219-migrateexception-chaning to hidden.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Needs review » Needs work

Back to NW. See my comment on the MR.