REF dev/core#2571 Move reCAPTCHA code to extension #20588
Conversation
|
(Standard links)
|
| public static function checkAndAddCaptchaToForm($formName, &$form) { | ||
| $addCaptcha = FALSE; | ||
| $ufGroupIDs = []; | ||
|
|
There was a problem hiding this comment.
@mattwire I think there are a few common patterns in this function definition, handled under each switch-cases, which can be avoided by:
- Removing all
if (!CRM_Core_Session::getLoggedInContactID())check by:
public static function checkAndAddCaptchaToForm($formName, &$form) {
if (CRM_Core_Session::getLoggedInContactID()) {
return;
}
...- Getting the $ufGroupIDs assignment in one place :
public static function checkAndAddCaptchaToForm($formName, &$form) {
if (CRM_Core_Session::getLoggedInContactID()) {
return;
}
$ufGroupIDs = method_exists($form, 'getUFGroupIDs') ? $form->getUFGroupIDs() : [];
...There was a problem hiding this comment.
@monishdeb I agree with those changes but that would change the existing behaviour which I'm trying to avoid for initial move of the code. As you can see there are inconsistencies - not all forms check if the contact is logged in for example when they probably should.
There was a problem hiding this comment.
Actually @monishdeb you were right - I've moved that to a "standard conditions" check.
mattwire
force-pushed
the
recaptcharefactor
branch
3 times, most recently
from
ed86a81 to
e0098b9
Compare
|
Other than tests the thing I worry about is interfaces and how stable they are if we are implying them. There are 2 interface-like aspects here
In the case of 2 the main concern is to protect us from would be copy & pasters & adding a comment warning in this section would address that. For the first however - I think we are setting up something people are going to expect to never change so I think we should decide what we want the interface to be. Potentially we should add a function to If we documented the agreed variant of that & added tests I think we could commit to keeping that working & not changing the signature at any future point @seamuslee001 @colemanw @demeritcowboy @totten @mattwire if you want to put the readme changes in a separate PR & add warnings to copy&pasters around using that function from extension code I think the bulk of this is mergeable if it doesn't imply any new supported interfaces (although obviously .... it really should have tests) - a big thanks to @homotechsual for testing |
|
@eileenmcnaughton At this point we are not talking about making this a standalone extension although that is the end goal. This is really still just refactoring to extract into a core extension which generally doesn't require the addition of tests and definition of external interfaces. I completely agree that they would be essential before making this a "standalone extension". The recaptcha extension is currently marked as "hidden" and, in my opinion, fully compliant with the original intent/contract for core extensions in that they would allow for gradually moving functionality to an extension without having to tackle some of the tougher, architectural issues such as interfaces. Regarding:
None of this work is changing that interface. I agree that we do need to define a supported interface before this becomes unhidden/standalone but feel that is out of scope of this part of the work. Also note that the end-goal here is to have "no reference" to recaptcha in civicrm core so I don't think adding a method to
I think this is a bit of a side-issue. Currently a lot of the control over displaying reCAPTCHA is defined by a flag in the profile but I'm not even sure this will be required in the future. Generally the display of reCAPTCHA seems to be based on "are you logged in?" and a generic setting for all public/anonymous forms is more likely in the future - especially for "hidden" form protection options such as honeypot / invisible reCAPTCHA etc. |
|
@mattwire I would disagree with you that " just refactoring to extract into a core extension" "generally doesn't require the addition of tests" - I have added tests to all the code I have refactored into core extensions. I feel a bit gaslit when you say that to be honest. Regarding whether defining an external interface is required - the trigger point for needing to define that in this case, as I mentioned was you writng a readme telling people how to add the code to their extensions - at that point you are defining a new interface. That function name could change at any time but by documenting it you are implying that it won't and that extensions can call it from outside of core - that is what an interface is. |
mattwire
force-pushed
the
recaptcharefactor
branch
from
e0098b9 to
c4adfa4
Compare
|
@eileenmcnaughton I see what you mean about the readme - I'd actually forgotton I'd written that level of documentation. Now updated to indicate that there is no supported method. Regarding tests, generally agree they're a good idea and you've done a good job. But looking at what we have as core extensions currently the "logic" ones such as sequentialcreditnotes and contributioncancelactions have a good set of tests. But the "UI" ones don't have tests - such as ckeditor4 and greenwich theme. I feel like ReCAPTCHA fits in the "UI" category and I don't see an easy way to test (indeed the logic of ReCAPTCHA is to prevent automated submission/testing) - the 7 forms which it is added to in core can be easily tested manually but validating that it is added using a test doesn't seem so straightforward. |
|
@eileenmcnaughton I meant to say I've updated the README now to be clearer. |
|
@mattwire I would personally have added tests to at least one form at either the point of extracting Regarding the readme update - I think it would be better to fully remove any 'how to' for interacting with the extension in an unsupported way from the PR template & the readme. The comment block for the function should have some suitably scary messages & internal tags added. On the scope and goal of the work - I can see that is still very much an open question on the gitlab issue so I won't get into it here - but it looks like it does need to be agreed in gitlab before more PRs |
mattwire
force-pushed
the
recaptcharefactor
branch
from
c4adfa4 to
7a1af4a
Compare
|
@eileenmcnaughton I've updated readme/comments/PR as requested. Think this should be ok now? |
|
Betty and I are reviewing PR's and we came across this one. We have asked @eileenmcnaughton in the chat for a comment on this PR as it looks like this PR is stalled. |
|
I have spoken with @eileenmcnaughton and she does not know how to move this forward. @mattwire do you think this PR is review ready? |
|
@jaapjansma Yes I think this is review-ready. There is a list of forms in the description of this PR where reCAPTCHA is loaded and these have all been tested "manually" to ensure that there is no change if this PR is merged. There are various concerns about reCAPTCHA in core that have been brought up:
To share my goal, I have a Work in Progress branch for https://lab.civicrm.org/extensions/formprotection which already contains logic for controlling which forms we should load reCAPTCHA (independent of the existing database tables). It will allow selection of reCAPTCHAv2 or v3. |
|
Test this please |
|
@mattwire the test failed. Can you have a look? |
mattwire
force-pushed
the
recaptcharefactor
branch
from
7a1af4a to
c1b477a
Compare
|
@mattwire did you change anything? Is this PR ready for review? |
|
@jaapjansma Yes I fixed the test failures. In my opinion it is ready for review |
|
Test this please |
@eileenmcnaughton or @colemanw can one of you merge this PR? |
jaapjansma
added
the
merge ready
|
jenkins retest this please |
|
Ah, thanks @demeritcowboy :-) |
Overview
This moves reCAPTCHA code to extension for all core forms:
Before
reCAPTCHA loaded via core form code.
After
reCAPTCHA loaded via extension code.
Technical Details
tpl files are still included via core templates at the moment.
Comments
@totten @monishdeb @eileenmcnaughton