Research alternatives to curl request on post apply event during cron updates

Why is this not core-post-mvp? 🤔

(I know far more about Automatic Updates now than at the time of #3 — I had only just begun working on it then!)

I think the thought was that it might be core-mvp to at least research the alternatives.

But I am not sure that it needs to be. If we don't get the PHP-TUF support required for Cron updates in the contrib module we really won't have any real world testers of the curl use because this only happens during cron updates. In the UI we make sure have a new request for PostApplyEvent by making sure the batch step runs in a new requests.

So basically will we really be able to tell if the current method causes problems?

Also our current non-test uses of PostApplyEvent are

1. UpdateDataSubscriber, which clears the update module data so the available updates page is correct
2. StatusChecker which clears the status check results after an update

both of these will happen for sure through the UI because we don't rely on curl

StatusChecker is cleared because some of the messages might be related to the old core version. But if the user sees this on admin page and click through to the status report the status checkers would be rerun and recached

UpdateDataSubscriber would me the available updates might not be correct after cron update but that page lists the last time it was checked and has a link to refresh.

I think one problem we could have if PostApplyEvent does not run correctly is that stage would not be destroyed so if you went back to Core Update UI it would tell you that there is existing operation. You could cancel it but you won't know that stage has already been applied.

We have no mechanism for determining if a non-destroyed stage has already been applied and is therefore safe to destroy. One reason for that is because we did think it might be possible valid use case for stage to be applied more than once.

For instance Project Browser could conceivably keep a stage around after a apply so they could install a new module without copying the whole codebase over again. The would have their own version of LockFileValidator and maybe decorate our version of LockFileValidator and return early if the stage was an instance of their installer stage.

I found #8 very hard to parse — it's kind of meandering in thoughts across 8 paragraphs, not presenting a clear answer to the question articulated in #7?

1. cURL may not always be available--especially on low end hosting or tightly secured self-hosted environments.

I think that this is the only core-mvp aspect: currently this module uses curl_init() etc but does not require ext-curl in its composer.json, and core doesn't yet either (although core requires plenty of other PHP extensions).

#3291147: Successfully applying an update puts the remainder of the request into an unreliable state → introduced the reliance on curl_*() functions and hence dependency on ext-curl. Devil's advocate: why is file_get_contents()/steam_get_contents() inadequate? 🤔

I updated #10 to be clearer(hopefully 🤷)

re #8 I created 📌 Document when PostApplyEvent should be used instead of hook_update_n() or post update RTBC

I think if we document that PostApplyEvent should be used for non-critical clean-up then the current issue becomes a little less important because in the worst case where a cron update is applied but PostApplyEvent does not get dispatched it will not be too bad

#11: thanks, much appreciated! Much clearer indeed.

First use of PostApplyEvent: clearing status check results after an update

What if the results are not just stored like they currently are:

    $this->keyValueExpirable->setWithExpire(
      'status_check_last_run',
      $results,
      $this->resultsTimeToLive * 60 * 60
    );

… but also keyed by the composer.lock's content-hash? That way, we'd prevent ever showing wrong information, and we should not even need to clear the stored status check results?

Second use of PostApplyEvent: refreshing update.module data

Can't we apply the same principle here? Decorate the update.manager service, where we'd override the constructor to:

1. retrieve the actual composer.lock's content-hash
2. compare this to the stored hash, and if it does not match, delete all cached data (I think by calling refreshUpdateData()?)

… everything else should be able to stay the same — it just means that we're adding an extra invalidation mechanism: the default mechanism only does time-based invalidation, we're just adding instantaneous invalidation when we know for a fact it should be updated. (Much like cache tags vs cache max-age!)

@tedbow and @tim.plunkett discussed the plan for getting Automatic Updates, Project Browser and Package Manager into core. We're likely going to aim to land Package Manager first. So postponing this issue for now, until the dust settles on that.

Package Manager alpha blockers are all done, unpostponing.

At #3341406-6: Document when PostApplyEvent should be used instead of hook_update_n() or post update → , it was explained in detail by @tedbow why we want to keep PostApplyEvent.

That means that during unattended (currently cron) updates, we still need to trigger it. And in HEAD as of today, CronUpdateStage:

1. runs the entire stage life cycle in a single request
2. and hence has to perform \Drupal\automatic_updates\CronUpdateStage::triggerPostApply() using curl to force \Drupal\package_manager\StageBase::postApply() to happen in a separate PHP process

3. But 📌 For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits Fixed is going to change that, so the risks are lower. However, we still want \Drupal\package_manager\StageBase::postApply() to be called as soon as possible after the apply operation is complete, because without its

       // Rebuild the container and clear all caches, to ensure that new services
       // are picked up.
       drupal_flush_all_caches();
   

   … the site will not be ready for use after the update, and we won't be able to turn off maintenance mode 😱 That's why the current solution was introduced in #3291147: Successfully applying an update puts the remainder of the request into an unreliable state → , to avoid 😄

   Fortunately, Drupal core has already solved this once before: update.php and the UpdateKernel. Which AFAICT has never been discussed:

   1. https://www.drupal.org/project/issues/automatic_updates?text=update.php&... → → 0 hits
   2. https://www.drupal.org/project/issues/automatic_updates?text=updatekerne... → → 0 hits

   (well, now that this is posted they'll each get 1 hit of course…)

   That was already forced to deal with complex edge cases: #2863986: Allow updating modules with new service dependencies → . Surely we should be able to adopt the same pattern for Automatic Updates?

   Note that it's \Drupal\system\Controller\DbUpdateController::batchFinished() which does both:

   1. drupal_flush_all_caches();
      

   2.     // Now that the update is done, we can put the site back online if it was
          // previously not in maintenance mode.
          if (!$session->remove('maintenance_mode')) {
            \Drupal::state()->set('system.maintenance_mode', FALSE);
          }
      

   … which is exactly what we're after.

   tl;dr: I propose first letting 📌 For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits Fixed handle updating the existing CronUpdateStage to work across multiple requests and then use this issue to convert it to use core's existing UpdateKernel and related infrastructure. If that fails, we should be able to subclass it to add the tweaks we need.

Per @tedbow at #3360656-7: For web cron updates run each stage life cycle phase in a different request → , 📌 For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits Fixed is now THE way forward.

So … AFAICT that means this can be tentatively postponed. Once #3357969 lands, I think this can be marked ?

#19 yep. assuming we can solve 📌 For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits Fixed the way we hope then we won't need this. I feel about 90% certain we can

We shouldn't need this because 📌 For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits Fixed seems to be working