(REF) dev/core#2571 - Add helper functions for reCAPTCHA extension

[mattwire]

Overview

There are a few simple checks that are used (inconsistently) on each form that adds a reCAPTCHA:

1. If it has a profile (UFGroup) check if the "add_captcha" flag is set on that group (currently it does this by merging that param into each profile field and then checking that - we simplify that and check the profile directly).
2. If the user is not logged in (anonymous) show the reCAPTCHA.
3. Some pages have additional checks (eg. create mode, forceRecaptcha).

This PR does not make any functional changes. It adds the function getUFGroupIDs(), removes unused hasToAddForcefully() and adds getIsCreateMode() to profile form.

Before

Different method required for each form to get a list of UFGroup (profile) IDs.

After

Single function getUFGroupIDs() can be called on all forms that implement reCAPTCHA and use profiles.

Technical Details

Comments

You can see how this will be used here: 94366a0

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

[eileenmcnaughton]

@mattwire @seamuslee001 @colemanw @totten - so this is an interesting one.

With core extensions the goal is that after a (potentially long) transition period they would only interact with core in supported ways. In this case the goal is to interact with the quickforms and these functions have been added to support that. However, we don't actually support any calling of quickForm functions from the quick form hooks (the hooks kinda exist but the contract for what you can do in them is clear as mud). We normally rely on a mixture of paranoia, unit tests & review to prevent too much breakage when extensions interact with forms.

The requirement here is to be able to retrieve the ufGroupIDs from the front end forms in a consistent manner - and I think the reason why that would be a good thing is fairly obvious.

I am wondering whether we should start to define some supported functions for extensions to access on quickforms - perhaps by adding an interface which then can implement and, if they do, then they would provide what would be a growing list of functions like this - thoughts?

[mattwire]

The interface between forms / extensions has always been rather messy. We standardised a bit when developing some of the entityForm stuff. Effectively we have a number of public getX functions defined on CRM_Core_Form which are much better than trying to access inconsistent properties on each form!

We could add:

/**
 * Get array of UFGroup (profile) IDs in use on the form.
 * return array
 */
public function getUFGroupIDs() {
  return [];
}

which would provide a level of consistency and encourage anyone adding support for (eg. captcha) to another form to implement that function.
I'm hoping that we can get (something like) this PR merged soon so that I can post the next step which removes quite a bit of code relating to reCAPTCHA from each form and reduces the interaction from the extension to just calling the functions included in this PR (see https://lab.civicrm.org/dev/core/-/issues/2571 / master...mattwire:recaptcharefactor ).

[eileenmcnaughton]

I've never regarded our sloppiness in core about using public as meaning that accessing those functions from anywhere outside of tested core code is supported - if we are going to establish some supported interfaces we should be deliberate about it - & document it.

I don't know if there is any code-way to demarcate other than being really clear in the comment blocks / developer docs

[mattwire]

Any extension that implements a quickform inherits from CRM_Core_Form so I think we have to assume that the public methods on that form are supported in some way at least. We are talking about an (incomplete) core extension at the moment so still fully under control of core.
What do you think of my suggestion to specify the function on CRM_Core_Form so it is "documented" for anyone interacting with CRM_Core_Form.
We don't have any documentation on interacting with CRM_Core_Form via extensions but do have things like civix which autogenerate code. Hoping that writing full documentation on interacting with CRM_Core_Form doesn't become a requirement here as that completely blows the scope and it's not been a requirement on previous additions to CRM_Core_Form - happy to work towards that but note that this PR is a significant improvement on before - ie. the question of how on earth do you get a list of profile IDs out of a form! I envisage the respective forms being able to use the functions in the future as well for further cleanup / simplification.

[eileenmcnaughton]

Well I'll wait & see what others think but I've always understood part of the process of converting stuff to core extensions being figuring out a supported way of doing the things that that they do in extensions - I'm guessing this will be the template for other recaptcha-type extensions so it's the guts of this project not a side issue.

I had been wondering about adding supported functions via a object (SupportedInterface) e.g

$form->getSupportedInterface->getUFGroupIDs();

I think we'd need to be somewhat strict about what could be added to it (eg only functions that return ids or apiv4 style result sets).

I guess we could add these functions if we code comment them to say it is NOT supported to access them outside of tested core extension code - then it would unblock you without in any way increasing what people expect us to support - that would punt the issue of a supported version down the road a bit

[seamuslee001]

@eileenmcnaughton @mattwire

As far as I was concerned as an extension developer I considered CRM_Core_Form at the very least to be a supported interface given the hook_civicrm_buildForm, hook_civicrm_validateForm etc hooks that all passed through the form.

I would also say that in terms of documentation we already have these pages which demonstrate a number of what I woulc consider as generally supported ways of interacting with Quickforms

https://docs.civicrm.org/dev/en/latest/framework/quickform/ https://docs.civicrm.org/dev/en/latest/framework/quickform/entityref/ https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_buildForm/ https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_postProcess/

[eileenmcnaughton]

@seamuslee001 yeah they do things like

    if ($form->getAction() == CRM_Core_Action::ADD) {
      $defaults['price_3'] = '710';
      $form->setDefaults($defaults);

& I agree those are super baked in - but that's different to assuming you can call functions on the form & they won't change

[eileenmcnaughton]

ie those are very much quick form functions they are calling - which we agree won't change since .... quickform

[seamuslee001]

@eileenmcnaughton getAction is well ours not quickform

civicrm-core/CRM/Core/Form.php

Line 1145 in 5db0bc3

return $this->_action;

and all those ExtendedTypes from the quickform documentaton https://docs.civicrm.org/dev/en/latest/framework/quickform/ are in CRM_Core_Form not actual Quickform (they may wrap around QuickForm ones but are ours) I would have also considered at least in my head that anything public in CRM_Core_Form has been fair game for Extension Authors to use, maybe not so much in the sub classes but certainly those ones as CRM_Core_Form to me has been our interface for form building in extensions

[eileenmcnaughton]

Hmm - yeah - we could maybe go through the form class & figure out which ones a legitimately supported for non-core use - a lot of them really DO relate the form itself - as opposed to this new front of adding functions that help people access individual form information. Most of those existing things on the base class are well tested already & are also simple + static

If we want to start supporting this new type of interface for use outside core I still think we need to be very deliberate about it - in terms of protocols, tests!! & documentation -

[mattwire]

@eileenmcnaughton @seamuslee001 I have added a docblock + added a skeleton method to CRM_Core_Form and provided documentation via https://lab.civicrm.org/documentation/docs/dev/-/merge_requests/915 - hopefully this is ok now?

[totten]

(matt) Effectively we have a number of public getX functions defined on CRM_Core_Form which are much better than trying to
access inconsistent properties on each form!

(eileen) Well I'll wait & see what others think but I've always understood part of the process of converting stuff to core
extensions being figuring out a supported way of doing the things that that they do in extensions

(etc etc)

Lots of good points in this thread so far -- esp about the extent to which CRM_Core_Form is (or is not) a supported interface. The discussion seemed to get fairly broad/general. Of course, it would be unrealistic to require a broad cleanup addressing all tensions for all Form methods in this one PR. I also really appreciated the screenshot Eileen posted because it shows the cumulative impact of these quirky methods. It does make sense to put some thought into doing new ones nicely.

IMHO, it's OK to expand the contract of CRM_Core_Form incrementally - but we should clarify more about the specific case
(recaptcha and ufGroupIds).

It was quite helpful reading 94366a0 to get some picture of how it would use getUfGroupIds. One observation (and this is no small thing): it does look like this refactoring is "safe" in the sense that existing
contrib users of CRM_Utils_ReCAPTCHA::enableCaptchaOnForm() would likely continue to function in the same way.

However, a few issues/questions stick out at me (in no particular order):

• @mattwire, I don't know the driver behind this work. Like: Is the ultimate goal to replace the captcha-provider (eg
  reCaptcha vs hCaptcha)? Or is the ultimate goal to change the activation-policy? Or is the intention to phase-out the
  captcha mechanism entirely (and phase-in some other antispam technique, like honeypot fields or email-verification)? Or
  some combination? Which (if any) changes are to be accomplished within this core-ext -- and which (if any) in
  other core-exts or other contrib-exts?

  (Without that context, I infer a goal: one moves captcha to an extension in order to make the captcha subsystem modular;
  i.e. to allow different consumers+providers and different site-customizations. This is fairly broad, so my comments have
  to be verbose+abstract to cover more ground.)

• If the goal is to be modular, then I would expect these subtopics:

  1. Implementation Provider: What service provides the captcha, and what elements (e.g. HTML tags, JS files, API keys)
     are required to load it? What steps does it need to run in (eg build-form, validate-form)?

     • I would interpret "modular" to mean: multiple extensions providing alternative implementations of "captcha" functionality.

  2. Activation Policy: Do we want to enable the captcha widgets on a specific form? Is that decision made by a developer when writing a subclass of CRM_Core_Form (or when writing MenuXML)? Or maybe it's a configuration decision in a UFGroup`? Or maybe an extension should define some kind of configurable rule-table? Or maybe we could simplify by skipping all the per-form stuff (just check the user's login/permissions)? Or... some hierarchical combination of the above?

     • I would interpret "modular" to mean: Each app-module (eg core's CiviEvent and contrib's CiviVolunteer) can choose its own default policy. Another power-tool can override these defaults.

  3. Placement: Given that we have decided to load some captcha widget on some form, where does it go? (While this is a distinct subquestion, I think we can gloss over it. It'd be "good enough" to always put the captcha elements at end of the form -- and it'd also be "good enough" to have consumers put a notation in the TPL file.)

• Based on reading 94366a0 (without full context), it looks like it conflates the implementation-provider with the activation-policy (ie ext/recaptcha fully controls both topics). I'm trying and failing to see how this refactoring makes it any more modular. (1: Other extensions - eg CiviVolunteer - won't be able to manage captcha the same way as CiviContribute/CiviEvent... because they can't patch CRM/Utils/ReCAPTCHA.php. 2: Replacing recaptcha with a contrib hcaptcha would be harder b/c hcaptcha needs to maintain a duplicated list.)

• There are qualitatively different ways to activate a UFGroup (on different screens) -- e.g. editing the single target-contact (new-contribution/event-registration/standalone-profile), or editing a related contact (on-behalf-of), creating new contacts in-situ (Contact Reference/Entity Reference), or updating a batch of contacts/records, or searching for contacts/records. The name suggests that any/all active UFGroups would be relevant, but the patch tunes the list to reflect captcha activation-policy. As soon as that method gets used for another purpose, we're liable to get boxed-in.

I'd love to understand this better before saying more, but I appreciate it's hard to coordinate, so excuse me in jumping-ahead
with a couple more thoughts:

• We could say, "getUfGroupIds() is expedient for building-out civicrm-core:ext/recaptcha,and we want to keep the cost low, so we'll look past any quirks or abstract debates." I'd be OK with that if we have a disclaimer that getUfGroupIds() is not supported for contrib usage.

• We could say, "The old activation-policy is weird -- and we should phase-in a new one." I'd probably agree with that but need more of the picture.

• We could say, "The goal is to define a supported contract for a modular captcha system." In that case, I think we should choose some signatures that more pointedly speak about captcha (e.g. $form->isCaptchaActive() and then... maybe a trait for the common case where new-contribution/event-registration/standalone-profile delegate down to UFGroup?).

[mattwire]

@totten thankyou for the review.

If the goal is to be modular, then I would expect these subtopics:
Implementation Provider: What service provides the captcha, and what elements (e.g. HTML tags, JS files, API keys)
are required to load it? What steps does it need to run in (eg build-form, validate-form)?

This part is effectively resolved. The extension has the parts necessary to load the reCAPTCHA are already in the extension (buildForm + a validate callback).

Activation Policy: Do we want to enable the captcha widgets on a specific form? Is that decision made by a developer when writing a subclass of CRM_Core_Form (or when writing MenuXML)? Or maybe it's a configuration decision in a UFGroup`? Or maybe an extension should define some kind of configurable rule-table? Or maybe we could simplify by skipping all the per-form stuff (just check the user's login/permissions)? Or... some hierarchical combination of the above?

This has always been a bit of a mess in core and is very inconsistent. Generally the activation policies are one or more of:

• User is not logged in.
• UFGroup/Profile has "is_captcha" field set to 1.
• We are on a contribution page and setting forceRecaptcha is set to "TRUE".
• Form specifically calls CRM_Utils_ReCAPTCHA::enableCaptchaOnForm($form);

I would like to make those more consistent / configurable but the intention in phase 1 is "move to extension with no functional change". Once it's in one place it becomes much easier to see the different activation policies.

Placement: Given that we have decided to load some captcha widget on some form, where does it go?

This part is not yet addressed and it's added manually by including a template in the form.
Currently form adds:

{include file='CRM/common/ReCAPTCHA.tpl'}

The intention of this PR is to get us to a point where we can move all the activation logic to the extension.

I have some ideas but have not yet explored in any detail how a third-party form might add a "captcha" provided by this extension. That is for the next phase!

Are we ok to merge this step so we can move forward?

[mattwire]

By the way we have https://lab.civicrm.org/dev/core/-/issues/2571 which is a better place for the wider architecture discussion

[eileenmcnaughton]

Just copying across my response from chat - if the goal here is just to get this merged - then the suggestion I made 2 months back would achieve that & at least arguably punt the need to add unit tests to the follow up PRs

#20315 (comment)

[mattwire]

Now updated with code comment:
* NOT SUPPORTED FOR USE OUTSIDE CORE EXTENSIONS - Added for reCAPTCHA core extension.

[eileenmcnaughton]

I think you missed the word 'tested' in your comments :-)