Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #3375337: only call civi in hook if handler enabled #894

Open
wants to merge 2 commits into
base: 6.x
Choose a base branch
from

Conversation

herbdool
Copy link
Contributor

Copy link
Collaborator

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applying this patch returns a fatal error on results page (if civicrm handler is disabled). To replicate:

  • Create a webform with civicrm enabled.
  • Submit a contact using the form.
  • Go to the results page.
  • Disable the webform_civicrm handler from webform settings.
  • Reload the results page.
  • Error.

@herbdool
Copy link
Contributor Author

@jitendrapurohit What was the error message? What is the desired result? I don't think it makes sense for the results that were submitted with webform_civicrm handler enabled, not to have some errors. Maybe just handle the error and display a message that the webform_civicrm handler is disabled? Or don't display anything for the civicrm-connected fields?

I think the error you found is better as a new issue. There has to be a way to truly disable the handler, which is what this PR attempts to do. How the results display deals with the civicrm handler being disabled seems like a new issue to me.

@jitendrapurohit
Copy link
Collaborator

@herbdool I mentioned the error because the PR aimed to fix "something" if the handler is disabled. I'm not sure what was the original issue this PR was trying to fix. Can you add more details pls?

Note: The above mentioned error replicates only after this change is applied and works fine without it.

There has to be a way to truly disable the handler, which is what this PR attempts to do.

Can you pls explain what is the exact use-case you're trying to fix? And the reason behind disabling the handler? what is the expected workflow after the handler is disabled? Do you still expect to see civicrm fields on the webform? If no, maybe a proper way to handle it would be to also disable all the civi fields from the webform?

@herbdool
Copy link
Contributor Author

@jitendrapurohit I'll get back to this soon. Basically, this supports migrating webform submissions. We can't have the migration triggering the handler. It should only be migrating and transforming via the migration script. If the handler is enabled during this migration (or if this hook is calling civi even if the handler is disabled) then webform_civicrm does all kinds of stuff assuming it's someone filling out a form.

I'll ask the reverse question: does it ever make sense to have handler that can't be truly disabled? If the handler is disabled on a form, nothing should trigger CiviCRM for that form. So the hook should respect that.

@herbdool
Copy link
Contributor Author

@jitendrapurohit I cannot replicate a fatal error. I do not get a fatal error when visiting the Results page (after submitting the webform), when I have applied this patch, and have the handler disabled.

Without the patch, I get this error:

Call to undefined function Drupal\webform_civicrm\civicrm_api3() in Drupal\webform_civicrm\Utils->wf_civicrm_api() (line 657 of modules/contrib/webform_civicrm/src/Utils.php).

But it loads fine otherwise. Can you test this again?

@jmcclelland
Copy link

@jitendrapurohit I am also unable to replicate the errors you are getting.

And I also need this patch so I can migrate our webform submissions from Drupal 7 to Drupal 10. I've now patched the webform_civicrm_migrate so it disables the webform_civicrm handler. However, without this patch it doesn't do any good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants