Skip to content

MMMM-206: Fix Deprecated Settings Usage#91

Open
shahrukh-compuco wants to merge 1 commit into
7.x-5.8-patchesfrom
mmmm-206-fix-deprecated-setting
Open

MMMM-206: Fix Deprecated Settings Usage#91
shahrukh-compuco wants to merge 1 commit into
7.x-5.8-patchesfrom
mmmm-206-fix-deprecated-setting

Conversation

@shahrukh-compuco
Copy link
Copy Markdown

Overview

This PR removes the deprecated contribution_invoice_settings usage and replaces it with actual individual settings.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the retrieval of CiviCRM settings by fetching individual settings such as 'invoicing', 'tax_display_settings', and 'tax_term' instead of the combined 'contribution_invoice_settings' array. Feedback suggests optimizing performance by caching these settings using static variables or object state to avoid redundant API calls within loops.

Comment thread includes/wf_crm_webform_preprocess.inc
Comment thread includes/wf_crm_webform_base.inc
Copy link
Copy Markdown
Member

@erawat erawat left a comment

Choose a reason for hiding this comment

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

Review: Approve ✔

Correct replacement of deprecated contribution_invoice_settings array access with individual setting lookups (invoicing, tax_display_settings, tax_term). Consistent with existing wf_crm_get_civi_setting() patterns in this codebase.

Minor notes (non-blocking)

  • SUGGESTIONwf_crm_webform_preprocess.inc:580: $taxTerm is fetched unconditionally but only used inside the if block on line 582. Could be lazy-loaded inside the condition for cleanliness (settings are cached so no real perf impact).
  • WARNING — PR description only has "Overview" section. The template requires Before, After, and Technical Details sections.

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.

2 participants