Document when PostApplyEvent should be used instead of hook_update_n() or post update

One reason to not use PostApplyEvent for things that absolutely have to run after an update is that this event is only dispatched for Automatic Updates not updates run in other ways. We already have hook_update_n() and post update hooks for this. Core also has a mechanism to determine if these functions were actually run. We don't need to rebuild that.

So you're thinking about a theoretical case where a contrib module would prefer implementing a \Drupal\package_manager\Event\PostApplyEvent subscriber over using hook_update_N() or a post-update hook?

P.S.: also see my proposal at #3318587-13: Research alternatives to curl request on post apply event during cron updates → which would likely allow us to remove all current PostApplyEvent subscribers. If you agree with my proposal there, we could potentially remove the PostApplyEvent altogether, or at least make it @internal for now? That'd eliminate this DX challenge completely!

For #3.

Ugghh. I was going to propose removing PostApplyEvent before because the need to do a second request in cron and in the console leads to a lot of complexity but then I realized we probably remove the 2nd second request anyways because it is probably important to send emails about cron update very soon after they happen, that would not work in my idea to get rid of PostApplyEvent

I will post it here anyways for now:

The not so great idea

Since we don't have concrete ideas about what PostApplyEvent should be used besides our own use case and we if release a 3.0.x version with PostApply or core version with this event it will be BC break to remove, we should just remove it now and add it back if need.

Right now our non-test subscribers are \Drupal\package_manager\EventSubscriber\UpdateDataSubscriber to clear the update data so the site shows there the correct available updates are a change and in \Drupal\package_manager\StageBase::postApply we call drupal_flush_all_caches(); so unless we have another solution to postApply() we would still need to do and we probably should not do it in the same request.

idea for how to replace current functionality we need

1. In \Drupal\package_manager\StageBase::apply we change the section to something like:
   

   \Drupal::state()->set('needs_post_apply', true);
       try {
         $this->committer->commit($stage_dir, $active_dir, $paths_to_exclude, NULL, $timeout);
       }
       catch (InvalidArgumentException | PreconditionException $e) {
   \Drupal::state()->set(StageBase::NEEDS_POST_APPLY, true);
         // The commit operation has not started yet, so we can clear the failure
         // marker.
   // We didn't actually make changes
   \Drupal::state()->delete(StageBase::NEEDS_POST_APPLY);
         $this->failureMarker->clear();
         $this->rethrowAsStageException($e);
       }
   

   We would set the state before the apply because afterwards it might not be possible if enough files have changed.

2. We add a new UpdateCacheClear subscriber service that subscribes to \Symfony\Component\HttpKernel\KernelEvents::REQUEST and checks the StageBase::NEEDS_POST_APPLY state and runs the 2 things we do now in UpdateDataSubscriber and also drupal_flush_all_caches();.

   it then would delete the StageBase::NEEDS_POST_APPLY state.

3. We could remove \Drupal\package_manager\StageBase::postApply.

   We would have to move the logic in \Drupal\package_manager\StageBase::setNotApplying directly into \Drupal\package_manager\StageBase::apply after the commit.

4. Delete the PostApplyEvent class and determine if need something like it in tests

Remaining tasks

Determine how to handle deleting stages in cron.

In form we can just delete the stage in the next batch step but in cron that won't work. Right now we delete the stage in \Drupal\automatic_updates\CronUpdateStage::handlePostApply but hopefully we can get rid of that function.

End of idea

what I realized at the end of this is that CronUpdateStage::handlePostApply is where we send the emails about the update that just happened. We probably don't want to do that in the same request because the code is in an indeterminate state so we want to get out of the request as soon as possible. So if we can't get rid of CronUpdateStage::handlePostApply and the need to make a second request getting rid of PostApplyEvent probably is not super important .

Also @phenaproxima on idea for the PostApplyEvent was that it would allow a contrib module that handle git commit/push after an update.

Do I understand correctly:

1. That we see legitimate needs for PostApplyEvent subscribers? Examples: notifying site owners, housekeeping tasks such as destroying the stage, infrastructure tasks such as generating git commits. Example of what should NOT happen: modify codebase, modify database (e.g. modules abusing this to apply DB updates). All of these should take a short time, if they don't, then it's up to that particular logic to handle queueing and running it a later time.
2. That we hence want to continue to support it as a public API and hence this issue remains relevant: we want to document acceptable uses and forbidden uses.

?

If so: 👍

Please also see the related comment at #3318587-18: Research alternatives to curl request on post apply event during cron updates → .

I gave a first try at the docs. I am sure they will need improvement.

I think this is a pretty good start, but I would change some phrasing. Here's the text I propose:

Dispatched after changes in the stage directory have been copied to the active directory. It should only be used for cleaning up after other operations that happened during the stage life cycle. For example, a PostCreateEvent subscriber might have set a state value which is no longer needed once the stage has been applied to the active directory -- in such a case, a PostApplyEvent subscriber should delete that
value. `drupal_flush_all_caches()` is called just before this event is dispatched, so subscribers shouldn't need to flush any caches or rebuild the service container. This event may be dispatched multiple times during the stage life cycle, and should *never* be used for schema changes (i.e., operations that should happen in `hook_update_N()` or a post-update function).

Looks clear to me. If we agree on the wording, then I guess this is good to go.

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