Afform - Remove ngRoute from afformStandalone page

[colemanw]

Overview

This removes an unused dependency from the Afform standalone page, making it more flexible and reusable.

Before

Afforms in "standalone" mode required ngRoute, limiting the usefulness of the Afform Standalone controller as it could only handle one afform per page.

After

Dependency removed.

Technical Details

Might need to do more to get AfformStandaloneCtrl working in multiple instances per page because ngApp is also limited to single-use. But this is a start.

[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]

• General standards
  • (r-explain) Soft Pass: Depending on how we deal with the r-run/r-tech comments below, we may want to update the description to note any transition steps.
  • (r-user) Pass
  • (r-doc) Pass
  • (r-run) Needs work:
    • ✔️ I ran this locally with a couple forms built in the GUI. Data-loading and data-saving seemed to work.
    • ✔️ Tried the deduper extension. It was fine. (It uses civicrm/a as its base page and then embeds some forms underneath.)
    • ⚠️ On afform_html and oauth_client, it had problems. These don't exactly use client-side routes, but they do use ?foo=bar to change UI state. Example steps to reproduce:
      1. cv en oauth_client
      2. cv open civicrm/admin/oauth
      3. Click a link like "gmail" or "ms-exchange". Observe that the URL updates but the screen doesn't.
• Developer standards
  • (r-tech) Mixed: It's technically a contract break. However, I don't think this part of the contract is widely. Grepping universe doesn't find any other published/affected extensions. If we can fix the instances in core and bubble-up some notes on the BC break, then it is mergable.
  • (r-code) Pass: I like how the JS setting (afform.open) is removed in favor of more direct invocation.
  • (r-maint) Pass
  • (r-test) Pending execution

A couple questions/thoughts:

• In this arrangement, what purpose does afformStandalone serve? It looks like an empty controller that does nothing. And -- if we're aiming to allow multiple elements -- then does the label "standalone" even make sense?
• For things like afHtmlAdmin.aff.json and oauthClientAdmin.aff.json, perhaps they can have an explicit opt-in (e.g. "requires": ["afformStandalone"] or "requires": ["ngRoute"], "client_route": "/"). Then all other forms would omit afformStandalone/ngRoute?

[colemanw]

Hmm, there ought to be a way to to handle query strings without needing a full router. I'll take another look at that.
And yes the nonfunctional controller can probably be removed depending on how we handle the query issue.

[eileenmcnaughton]

2 test fails relate

api_v4_AfformRoutingTest.testChangingPermissions
api_v4_AfformRoutingTest.testChangingPath

[colemanw]

Joomla does not use $_GET['q'] which is why we have a function for this.

[colemanw]

@totten I think this is done and should be merged now as I've addressed your feedback.

However, after merging this I think we should have a dev meeting about afforms and url params and the potential pitfalls of having more than one afform on the screen.

[totten]

Cool, I did a new r-run on the test site -- and the hyperlinks are working in oauth-client and afform-html. Merging.

Agree on both comments.

On having multiple subforms that consume params - yeah, that can be messed up. It's not strictly problematic like with routing -- you could design the page params to either match-up purposefully or to avoid naming conflicts. It's conditionally problematic - if there's an accidental naming conflict.

We've been following a pattern to date where a form opts-in to different media (eg server_route, is_dashlet, is_token). I think this helps limit the risk. For example, oauthClientAdmin uses a param and is less amenable to mixing -- so it should have is_dashlet=0, and it should not be offered in the mixable dashboard context. If we wanted a further safety rail, we could sprinkle in some advice/help in the GUI editor ("Warning: This form enables dashlet mode and URL parameters... yadda yadda...")

I have been wondering how we should present the toggle for WP shortcodes, eg

1. Use is_dashlet=<bool> for Civi dashboard and is_shortcode=<bool> for WP (and maybe a future is_block=<bool> for Drupal/Backdrop)
2. Use is_dashlet=<bool> -- to enable any/all dashboard-style/mixable contexts (Civi dashboard and/or WP shortcode and/or Drupal block -- depending on local env). This maybe reduces clutter for admin UI.
3. Use media=[dashlet,shortcode,block,token,...] -- a multiselect. It's easier to add/remove options in the future without changing schema.

Might be a good time to settle that since we have two output media developing (mail-tokens #19660 and WP shortcodes).