Afform - Require Authx

[colemanw]

Overview

Form Builder (Afform) depends on functionality provided by the Authx extension. This adds it as a requirement.

Before

Afform doesn't require Authx but if you don't install it, email tokens won't work, and neither will form submissions in some cases.

After

Required.

[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).

[totten]

What happens if you have (say) CiviCRM 5.48 with authx=disabled and afform=enabled then upgrade to this?

[colemanw]

This is what happens:

[totten]

OK, so I ran through a few upgrade paths from 5.45 (starting with various permutations of afform={on,off}, authx={on,off} and CiviGrant={on,off}). Basically, they were OK, but I have few notes.

1. In the case where we need the status-message (eg you start with startVer=5.45, afform=on, authx=off, CiviGrant=off; then see "Form Core... missing dependency..."), I did see the status-message. 👍 However, the page-footer incorrectly reported "System Status: Ok". Investigated and split off Status Check - Report the overall status (accurately) #24027.

2. An interesting scenario: startVer=5.45, afform=off, authx=off, CiviGrant=on It will enable civigrant and then (transitively) enable both afform and authx. I wasn't thinking about CiviGrant (initially), so I didn't expect this, but it's an appropriate outcome. 👍 (I would guess that an upgrade from 5.47 or newer wouldn't be quite as clever -- it would just get the same status warning as above.)

3. However, there were some quirks in that scenario. I did a few trials (eg with CLI and with web UI) and noted quirks along the way:

   • With CLI: The system-status wouldn't render until I ran cv ext:upgrade-db. (But I think this is a pre-existing issue; not relevant to this PR.)

   • With web UI: After finishing the upgrade, the dashboard showed a warning:

     Warning: in_array() expects parameter 2 to be array, null given in Civi\Afform\StatusChecks::hook_civicrm_check() (line 53 of /Users/totten/bknix/build/dmaster/web/sites/all/modules/civicrm/ext/afform/core/Civi/Afform/StatusChecks.php).

   • With web UI: Again, the system-status wouldn't render until I ran ext upgrade via civicrm/admin/extensions. (Same as above.) But after running the update, the system-status showed an unexpected warnings:

     Email token support has been configured for 1 form(s). This requires JWT authentication, authx_auto_cred does not include JWT.

     But this warning clears up after another system flush.

   • I think these last two warnings are the same problem: the default values for authx_* settings aren't initially loading, so they appear as NULL. Any assertions about authx_* settings then get (temporarily) weird/unexpected data. But once you reload metadata, the defaults get cleared up.

4. In one pass at r-run, I seemed to get stuck in a condition where the system-status wouldn't render, and cv ext:upgrade-db wouldn't execute. (There was a managed-entity conflict blocking it.) However, I haven't been able to reproduce it since. It could've been a mistake/fluke in my testing. But if anyone else gets it, then maybe it merits a closer look.

I'll fire a new Jenkins run and give it "merge ready" because I don't think these issues are the fault of this patch. However, I'm not really certain about (3) -- the authx_* settings could be a new issue.

[totten]

jenkins, test this please

[totten]

FWIW, when I combined this with #24030, it seemed to resolve the issues under (3).

[colemanw]

@totten that looks good.
FYI I think simply displaying a system status warning is fine in this case because the Afform dependency on AuthX is "soft" (i.e. doesn't cause a crash if missing). However, if we were to introduce a harder dependency, then the site might not even be functional enough to see a system status message. Something to ponder for the future...