[SD-148] configure captcha in Tide webform #111
Conversation
vincent-gao
force-pushed
the
feature/SD-148-Add-ability-to-configure-captcha
branch
12 times, most recently
from
31e1eda to
4660398
Compare
vincent-gao
requested review from
anthony-malkoun,
MdNadimHossain and
yeniatencio
vincent-gao
force-pushed
the
feature/SD-148-Add-ability-to-configure-captcha
branch
from
4660398 to
be5a3a1
Compare
|
The test failed is due to https://github.com/dpc-sdp/dev-tools/blob/master/tests/behat/bootstrap/TideCommonTrait.php#L138 is surprisingly not compatible with Claro |
vincent-gao
force-pushed
the
feature/SD-148-Add-ability-to-configure-captcha
branch
from
c10fabe to
5a5070c
Compare
There was a problem hiding this comment.
@vincent-gao some minor fixes would be good. Let me know if the comments make sense.
tide_webform.module
Outdated
| } | ||
| catch (\Exception $e) { | ||
| \Drupal::messenger()->addWarning('Unable to load CAPTCHA type options. Please contact the site administrator.'); | ||
| \Drupal::logger('tide_webform')->error('Error loading CAPTCHA type options: @message', ['@message' => $e->getMessage()]); |
There was a problem hiding this comment.
Should this logger include the webform id as well for better debugging when looking into the logs. May be something like this -
\Drupal::logger('tide_webform')->error('Error loading CAPTCHA type options for webform @webform: @message', ['@webform' => $webform->id(), '@message' => $e->getMessage()]);
There was a problem hiding this comment.
changed. thanks.
| $captcha_type = NULL; | ||
| $allowed_values = []; | ||
| try { | ||
| $field_definition = \Drupal::entityTypeManager()->getStorage('field_storage_config')->load('taxonomy_term.field_captcha_type'); |
There was a problem hiding this comment.
Consider refactoring these calls to reduce repeated service lookups. It is been used 4 times in this function. May be define it in a local var and use it to avoid this
There was a problem hiding this comment.
hey @MdNadimHossain , Could you clarify what you mean by "4 times" in this function? I’m unable to find any others. There are 2 other similar services using \Drupal::entityTypeManager()->getStorage('taxonomy_term'), but they retrieve the taxonomy_term storage, not the field_storage_config storage.
I’ve updated the other two to use a variable like this:
$taxonomy_storage = \Drupal::entityTypeManager()->getStorage('taxonomy_term');
There was a problem hiding this comment.
oh sorry, my bad. I meant the call for entityTypeManager()
so something like this -
$entity_manager = \Drupal::entityTypeManager(); $field_definition = $entity_manager->getStorage('field_storage_config')->load('taxonomy_term.field_captcha_type');
tide_webform.module
Outdated
|
|
||
| $form['third_party_settings']['tide_webform_captcha']['captcha_type'] = [ | ||
| '#type' => 'select', | ||
| '#title' => 'Captcha type', |
There was a problem hiding this comment.
changed. thanks.
tide_webform.module
Outdated
| $form['third_party_settings']['tide_webform_captcha']['enable_captcha'] = [ | ||
| '#type' => 'checkbox', | ||
| '#default_value' => !empty($third_party_settings['enable_captcha']) ? $third_party_settings['enable_captcha'] : NULL, | ||
| '#title' => ('Enable captcha'), |
There was a problem hiding this comment.
changed. thanks.
vincent-gao
force-pushed
the
feature/SD-148-Add-ability-to-configure-captcha
branch
from
b519fea to
628299a
Compare
|
hey @MdNadimHossain , thanks for the CR, I’ve made some updates based on your feedback. please take a look. |
There was a problem hiding this comment.
Thanks for the changes @vincent-gao
LGTM, resolve the conflicts before merging
try the test patch changes based on the feedbacks
vincent-gao
force-pushed
the
feature/SD-148-Add-ability-to-configure-captcha
branch
from
628299a to
f9d29d2
Compare
vincent-gao
deleted the
feature/SD-148-Add-ability-to-configure-captcha
branch
Jira
https://digital-vic.atlassian.net/browse/SD-148
Changes
Integrate
tide_webform_captchasettings into webforms to allow each form to configure its own captcha types.Related PRs