Problem/Motivation

while working on test in #3254207: Only allow 1 patch release update increment in Cron with @kunal.sachdev we noticed the validation results for cron update attempts are lost during the error logging.

We do

try {
      $this->begin([
        'drupal' => $recommended_version,
      ]);
      $this->stage();
      $this->apply();
      $this->destroy();
    }
    catch (\Throwable $e) {
      $this->logger->error($e->getMessage());
      return;
    }

So if the Update throws an UpdateException we only get the generic "Unable to complete the update because of errors." message.

Proposed resolution

If aStageValidationException is caught during a cron update attempt log all everything in getResults()

Issue fork automatic_updates-3259810

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

tedbow created an issue. See original summary.

kunal.sachdev made their first commit to this issue’s fork.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

@kunal.sachdev just some notes here from our discussion.

To test the message logged.

  1. Add a testErrors() to \Drupal\Tests\automatic_updates\Kernel\CronUpdaterTest()
  2. You could use a dataProvider that has 2 arguements, array $validation_errors, string $expected_log_message.
  3. Use TestChecker1::setTestResult($validation_errors, PreCreateEvent::class); to force an error in the Updater during cron. (see \Drupal\Tests\automatic_updates\Functional\UpdaterFormTest::testUpdateErrors)
  4. Check that $expected_log_message is logger when cron is run. (see \Drupal\Tests\package_manager\Kernel\StageOwnershipTest::testStageDestroyedWithFileSystemError for testing that log message exists. I think we also started this in #3254207: Only allow 1 patch release update increment in Cron)
  5. Then assert no update was attempted. We can probably move \Drupal\Tests\automatic_updates\Functional\UpdaterFormTest::assertUpdateStagedTimes() to a shared trait for this.

kunal.sachdev opened merge request !179

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Active » Needs review

phenaproxima made their first commit to this issue’s fork.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Although I contributed to this, I mostly just did minor cleanup. Therefore I feel okay RTBCing this.

  • tedbow committed a623c25 on 8.x-2.x authored by kunal.sachdev 
    Issue #3259810 by tedbow, kunal.sachdev: Validation errors are not...
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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