MigrateExecutable should catch not only exceptions, but also fatal errors

Fixing test cases!

The Novice task has been done, so I am removing that tag.

It is really annoying that \Error does not correspond to 'error', but I do not think we can do anything about that (unless we go through a deprecation process). If we want to do that, it can be in a follow-up issue.

It is not too late to replace 'fatal' with 'critical', to match const MESSAGE_CRITICAL. Let's do that.

I am adding the tag for an issue summary update. Update the Proposed resolution section to match the current patch. (At least the name of the constant needs to be updated.) That seems like a Novice-level task, so maybe I will not remove that tag after all.

If we have explicit steps to reproduce, then we should be able to create a test. Probably it will have to be a functional test, not a real unit test.

Added a patch which replace the 'fatal' as mentioned by #13.

So, I'm going to go on the record as being opposed to this change. A quick search through core for /catch\s?\(.*\\Error\s.*\)/ tells me that nowhere else in Drupal core do we attempt to catch php errors, and for pretty good reasons. Having to trap errors at this level suggests that there are significant flaws in the migration system that should be addressed. There shouldn't be a way to write a custom migration using only core code that is capable of intentionally triggering a fatal error, unless you have a test module with a custom plugin that is intentionally doing something bad.
If there is a way to write a custom migration that triggers a fatal error that isn't handled by the migration system before it happens, we should fix that, not try to hide it.
If we have to write a broken custom plugin to test this, then we aren't really testing core code.
There are plenty of processes in core that would be left in a funky state if a fatal error occurs during them, and core passes off to uncontrolled contrib plugins all the time, so why should this particular system be the one instance where we try to catch a Fatal error?

I understand the reasons for wanting to do this, and at the end of the day, I'm happy to be overruled, but to me, this is not good coding. We shouldn't be catching errors, we should be programming defensively to make sure they don't occur.

Adding in a relevant thread from the recent migration meeting:

4️⃣ MigrateExecutable should catch not only exceptions, but also fatal errors

Participants:

benjifisher, mikelutz, xurizaemon

A quick search through core for /catch\s?\(.*\\Error\s.*\)/ tells me that nowhere else in Drupal core do we attempt to catch php errors

… because this only became possible in Drupal 9, which hasn't existed for very long, since it requires PHP >=7.

Drupal 9 has been branched off for over a year at this point. Plenty of time to trap one error somewhere if that was what we were going to do.

Having to trap errors at this level suggests that there are significant flaws in the migration system that should be addressed.

Any migration source/destination/process plugin can trigger a fatal PHP error that can crash an entire migration. I'm arguing it should not.

This isn't compelling to me. ANY plugin(/hook/controller/event handler/theme function) that executes code can crash an entire page load or any other process that drupal manages. This is not unique to migrate plugins, yet you propose a unique solution.

If there is a way to write a custom migration that triggers a fatal error that isn't handled by the migration system before it happens, we should fix that, not try to hide it.

Can you point me to the places in the codebase and to issue queue discussions where this is an explicit rationale?

You want me to find precedent for the idea that we should fix errors rather than hide them?

If we have to write a broken custom plugin to test this, then we aren't really testing core code.

Core code is generally more robust than contrib/custom code, so IMHO this is not a convincing justification.

This isn't a justification, it's simply a fact. You would be testing a reaction to code using the core apis that is intentionally broken, and not just broken in that it is using the core apis in an unintended way (which we SHOULD be programming defensively for) but for code that decides to write $x = $y / ($z-$z); which has never been something we have previously attempted to save the user from.

We shouldn't be catching errors, we should be programming defensively to make sure they don't occur.

Agreed :) But we can't control all code that all people write. And I think this very measure would fall under the umbrella of "programming defensively"? 😅🤓

I do agree that we can't control all the code that all people write. Drupal's policy has always been to use contributed modules at your own risk. Our job is to provide a rock-solid core experience, and a robust api, and tools that contrib maintainers and contrib contributors can use to monitor their code and have issues reported. If core is going to be responsible for all catchable errors that contrib code might possibly cause, then I think that's a bigger discussion, not something we pioneer in the migration system. If this is the policy we are going to go with then we need to start wrapping any hook or event or plugin calls or anywhere else that can call custom or contrib code with try/catch(\Throwable). If we decide to go that route as a community, so be it, but I just don't think it's right to try tarbitrarilyly do it here.

Code review below - we need to be really sure that we want to do this before going ahead, particularly because there's a breaking API change for anyone that has sub-classed MigrateExecutable and handles exceptions differently. Yes, there's the 1:1 rule, but this would definitely be 9.1 only because of a breaking change - and we'd have to be reallllly sure we wanted to do it. Personally, I've been in the scenario where this has caught me out whilst I'm working on developing a new source/process plugin - but if someone is in that scenario - they are comfortable interpreting PHP errors, and resetting the migration status.

So, tl;dr I don't feel strongly either way (sorry).

1. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -187,9 +187,9 @@ public function import() {
   +    catch (\Exception | \Error $e) {
   
   @@ -217,6 +217,11 @@ public function import() {
   +      catch (\Exception | \Error $e) {
   
   @@ -244,7 +249,7 @@ public function import() {
   +        catch (\Exception | \Error $e) {
   
   @@ -267,10 +272,10 @@ public function import() {
   +      catch (\Exception | \Error $e) {
   
   @@ -436,19 +441,20 @@ public function saveMessage($message, $level = MigrationInterface::MESSAGE_ERROR
   +   * @param \Exception|\Error $exception
   

   sidenote: we could just use \Throwable here as it's a common ancestor of both of these?

2. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -436,19 +441,20 @@ public function saveMessage($message, $level = MigrationInterface::MESSAGE_ERROR
   -  protected function handleException(\Exception $exception, $save = TRUE) {
   +  protected function handleException($exception, $save = TRUE) {
   

   this is an api change, and if we changed the typehint to \Throwable, it doesn't help - see https://3v4l.org/RlS0n and https://3v4l.org/rIOsE

Why does try/catch have to be added to the executable? Why couldn't it be added to the UI or batch form that is calling the executable? That's how we handled it in migrate_tools, where we've added a --continue-on-failure flag to do sorta what is proposed here.

+++ b/core/modules/migrate/src/MigrateMessage.php
@@ -17,6 +17,7 @@ class MigrateMessage implements MigrateMessageInterface {
+    'critical' => RfcLogLevel::CRITICAL,

This might seem like a small thing, but adding an additional level of logging to the migrate message could be a BC issue. We've had issues before in migrate tools; I assume in migrate run as well. Could we remove the logging level to a follow-up so we don't bikeshed on it?

The rest of my concerns are pretty well addressed. I'm still a little concerned about continuing to run the migration once it hits errors. What if someone has logic built-in to their execution to handle them. But let's see what other have to say.

re #28.1: Drush uses different logging level and mapping. See https://gitlab.com/drupalspoons/migrate_tools/-/blob/8.x-5.x/src/Drush9L.... We also have similar logic added to migrate_upgrade and other migrate contrib modules.

re #28.2: If anything can be done, then it probably will be done in Drupal. I'm probably being overly cautious, but https://xkcd.com/1172/ seems appropriate.

AFAIKT this issue is not specific to Drupal 7 sources, removing tag.

The proposed resolution in the issue summary needs an update. It should emphasize that this is proposing a change in behaviour in MigrateExecutable and explain the pros and cons of it. Especially, what are the uses cases where continuing despite fatal errors is an improvement. It should also explain what happens to the state of the migration when a fatal error occurs.

What stands out for me are these comments, all of which I agree with.

heddn: "I'm still a little concerned about continuing to run the migration once it hits errors."
mikelutz: "If core is going to be responsible for all catchable errors that contrib code might possibly cause, then I think that's a bigger discussion"
larowlan: "Personally, I've been in the scenario where this has caught me out whilst I'm working on developing a new source/process plugin - but if someone is in that scenario - they are comfortable interpreting PHP errors, and resetting the migration status."

And I am not yet convinced that using the migrate message tables to log fatal errors is the right thing to do.

I then tested this when using the Migrate UI. I made a divide by zero error in the process plugin, FieldInstanceSettings, when the field was a term reference. Without the patch the Ajax error (screenshot below) error is displayed and the migration is halted. Looking in the log, the latest error in the divide by zero error so it is easy for the user to find. With the patch the migration continues on after the fatal errors and the success message is displayed. I looked at the log (/admin/reports) and, of course, the divide by zero error was not there. The user has no feedback that there was an error. So, at best, the patch needs work for that.

Now, we know that the error message is in migrate_message_d7_field_instance but how will the user of the UI find it? Migrate tables are not accessible via the UI and even if they use something like PhpMyadmin, they would have to search all the message tables for errors. And then even if they do find it only contains, 'divide by zero', which clearly lacks the information needed to track down the problem.

All of which means that I do not agree with this proposal.

Still, as chx often said, 'migrate is a special snowflake'. Perhaps the error could be caught so that state could be changed to indicate a fatal error has occurred instead of IMPORTING. However, I do not want a behaviour change, I would still expect the migration to be 'stuck' in this state until some action is taken to un-stick it. That would prevent an accidental actions on the migration, such as ''mim --update' or rollback. Even with that suggestion I am still concerned about setting a precedent of capturing errors and what others may think that means for other parts of core.

Without the patch

Watchdog message without the patch

With the patch

Logged message in message table with the patch. There is no watchdog message.
1 eef7ce1a543f5239383e7a7e256d5d4b78706b000306068229... 4 Division by zero

As of #33

@Anybody, thanks. I left this at Needs Review because there is not an agreed upon path forward so working on the patch isn't needed now.

Let's see if I can make the clearer in the IS.

The patch does no longer apply in 9.3.x, so I re-rolled it.

This was brought up at #3247150: [meeting] Migrate Meeting 2021-11-04 2100Z and the maintainer present, benjifisher, mikelutz, and quietone, agree to close this as won't fix after the meeting, assuming no further comments. Since there hasn't been I am closing this.

See the Remaining Tasks section in the Issue Summary for links to the relevant comments.

Add link to an earlier migrate meeting where this was discusses.