Afform - Provide default page_route for every form #31834
Conversation
Previously if a form did not have a page route it was inaccessible from any URL.
Now a default url exists for every form: `civicrm/afform/view/{afformName}`.
This is similar to the way Drupal nodes all have a default url of `node/{nid}`.
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
Aaah, yeah. We live in a world with different types of forms. From a UX POV, the most common case of "New Form" would be like "New Submission Form", and you almost always want an HTTP route for that. Embeddable things -- like dashlets and subforms -- are valid but probably less common. Thinking out loud....
Anyway, I'll still do some |
| @@ -1,5 +1,10 @@ | |||
| <?xml version="1.0" encoding="iso-8859-1" ?> | |||
| <menu> | |||
| <item> | |||
| <path>civicrm/afform/view</path> | |||
There was a problem hiding this comment.
@totten just to clarify your questions about server_route, the heart of the PR is right here, where we add a new generic menu route that handles all afforms. Going to this route and appending the name of a form (e.g. civicrm/afform/view/myCoolForm) will load that form.
There was a problem hiding this comment.
Aaah, OK. That interpretation is clearer.
| $formName = $pageArgs['afform'] ?? NULL; | ||
| if (!$formName && $this->urlPath[0] === 'civicrm' && $this->urlPath[1] === 'afform' && $this->urlPath[2] === 'view') { | ||
| $formName = $this->urlPath[3]; | ||
| } |
There was a problem hiding this comment.
@totten the other half of the heart of the PR is here, where the afform base page is routed both from the form-specific server_routes, and also from the generic route. This patch adds handling for the generic route (when $pageArgs is empty).
FYI to clarify your question about the navigation menu and conditionality, I think you may have misread that bit. No it's not conditional. The generic route is available for every form.
Effectively that means that if you give your form a server_route, it is now availble in two places.
This is the same as for e.g. drupal nodes, where if you give it a vanity url it is now available a /my-cool-page and it is also still available at /node/123.
The rest of the PR is just updating the GUI to account for the availability of the generic routes.
| if (!empty($afform['navigation']) && !empty($afform['server_route'])) { | ||
| if (!empty($afform['navigation'])) { |
There was a problem hiding this comment.
@totten maybe this is what you interpreted as being conditional. What's happening here is I'm removing the condition.
Previously a nav menu could only be added for Afforms with a server_route defined. That's no longer the case, since now every afform has a route, so nav menu functionality can be made available unconditionally.
|
@totten I take your point about security/privacy. We have no way to unpublish a form, so if a url route is permanently enabled, we get overexposure. Maybe your assumption that a default route is provided by the gui is the safer path forward... although that too could be problematic if the user doesn't notice the default being filled in for them. But at least it wouldn't expose all previously unexposed forms. I'm less concerned about forms that shouldn't have a url having one. Afforms that are not meant to be viewed at a page route (blocks, subforms, etc) will be non-functional or nonsensical at the default route, but so what? No one will ever have a reason to go to those urls. |
Yes, well put.
Sort of. I mean, if you're probing a system for weaknesses, then that's exactly the kind of URL you go to. The behavior is undefined. As in: it does "something", but there is no "sensible" thing to do. Whatever arbitrary behavior it has today would quietly change another arbitrary behavior in two months. I'm not trying to make a hassle, and I don't want to dig in heels against the wildcard path. Like if there's a good reason we need the complexity, then OK. But from where I'm sitting, it seems like a simpler patch (e.g. totten@332bd9f) does the job. Or, maybe with a bit polish, it might be like #31843 |
|
@totten the more I think about it & discuss it, the less I like this PR so I'm going to close it. But really it's not that much hassle and so for now I'm just going to make it required: 8996df2 That takes the pressure off this & we can slow down and think about whether we really want it. |
Overview
Ensures every afform is accessible via page route. This allows any afform to be used in a link or popup dialog without requiring the user to configure a route.
Before
Previously if a form did not have a page route it was inaccessible from any URL.
After
Now a default url exists for every form:
civicrm/afform/view/{afformName}. This is similar to the way Drupal nodes all have a default url ofnode/{nid}.FormBuilder UI updated to show that a custom route is optional, and a form can still be viewed in a page or popup or added to the navigation menu. If blank, the placeholder shows the default route: