dev/core#1674: fix rebuilding of container on every request

[jaapjansma]

Overview

The civicrm container should be cached and build once. However if an extension is installed and enabled and it does not contain the directories Civi/Api4/Event/Subscriber and Civi/Api4/Service/Spec/Provider the container is rebuild upon every request.

This ia non-functional change.

Before

The civicrm container is rebuild upon every request.

After

The civicrm container is loaded from cache.

How to test

1. Install an extension which does not contain the mentioned directories. E.g. civirules
2. Check whether CiviCRM still works.

Comments

This fixes API4 functionality in CiviCRM Core. I will also provide a separate PR for the api4 extension. However I am also convinced that this should not be in core at all.

See also https://lab.civicrm.org/dev/core/-/issues/1674

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

Since this PR does alter executable source code (and as you said yourself in the description "This fixes API4 functionality in CiviCRM Core"), I'm removing the NFC label. See these PR docs for more on the subject, and please let me know if they are unclear.

[jaapjansma]

@colemanw could you do PR review?

[eileenmcnaughton]

@jaapjansma if this is a regression (as you indicate) it should go against the rc branch. Having said that it's probably better to wait for @colemanw to take a look at the best place to cache this.

[eileenmcnaughton]

@colemanw can you look at this before the next rc is cut?

[jaapjansma]

I have submitted a new PR against the 5.23 branch, is that wou meant against the RC?
See: #16934

[colemanw]

I think this makes sense but I'd like to get input from @totten

[totten]

Ooooh. I was a bit confused at first because nothing in the previous code directly suggested that it would rebuild non-existent directories. But digging into DirectoryResource.php confirms the matter:

    public function isFresh($timestamp)
    {
        if (!is_dir($this->resource)) {
            return false; // To wit: a non-existent directory will force a rebuild
        }

I'm giving this some r-run to see if other cache update scenarios work the way this is expected to. (To wit: if you create the directory and add files, then it should update. If you delete the directory or files, then it should update. But if the directory/file-listing is stable, then it should remain cached.)

[jaapjansma]

Regarding when a directory is added/removed in an extension this would probably happen when someone updates an extension and a cache clear is a common action to do after updating extensions. And I believe that it does this automatically

[jaapjansma]

Thanks @totten for doing this more in a symfony way!

[eileenmcnaughton]

@totten I'm on the fence about master vs rc for this - I would lean rc but it's so close to release date - however, if it feels very safe to you I would go rc

[totten]

@jaapjansma Cheers :)

Yeah, there's a little known setting CIVICRM_CONTAINER_CACHE which determines the caching behavior. With the extra commit, I think CRM_Api4_Services will be compliant with the setting . That setting allows 3 modes:

• always => Always use the container cache, even if it's stale. Updates only come when there's an explicit flush. This is the best option for performance, but worst for DX, and slightly more brittle with certain extension-updates.
• never => Never use the container cache. This is best for DX but worst for performance.
• auto (current default) => Monitor the timestamps of relevant files. Update container cache if timestamps change. This is almost the best of both worlds (with small compromises on perf and DX)... as long as the FileResource/DirectoryResource stuff is setup well. (A problem like dev/core#1674 certainly spoils it.)

@eileenmcnaughton - I view it as moderately safe after running a few different use-cases. My initial gut would be master (5.25 / May) but I'm not opposed to 5.24 (April).

[eileenmcnaughton]

@totten I think either is fine since it's not super recent - tests have passed - shall we merge?

[colemanw]

@totten to address this question, the above try/catch was added in #15765 specifically to address the directory not existing. That was its only purpose so if it's now redundant I would say yes this can be removed.

[totten]

@colemanw Cool. I've cleaned up that bit and re-tested.