Add manual extension #401
Add manual extension #401
Conversation
WalkthroughThe changes in this pull request introduce the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
wp-content/civi-extensions/civirazorpay/civirazorpay.php (2)
1-5: LGTM! Consider adding strict types declaration.
The file setup follows good practices with proper dependency management and namespace aliasing. However, consider adding strict types declaration for better type safety.
Add this line after the PHP opening tag:
<?php
+declare(strict_types=1);
require_once 'civirazorpay.civix.php';1-32: Consider implementing additional essential CiviCRM hooks.
While the current hooks are well-implemented, consider adding other essential CiviCRM hooks that might be needed for a payment processor extension:
hook_civicrm_managed: For managing payment processor typehook_civicrm_navigationMenu: If you need to add menu itemshook_civicrm_entityTypes: If you're defining custom entities
Would you like me to provide implementations for these additional hooks?
wp-content/civi-extensions/civirazorpay/managed/PaymentProcessorType.mgd.php (2)
47-51: Improve credential label clarity
The current labels for API credentials could be more descriptive to help administrators understand what credentials are needed.
Apply this diff to improve clarity:
- 'user_name_label' => 'apiKey',
- 'password_label' => 'secret',
- 'signature_label' => 'signature',
+ 'user_name_label' => 'Razorpay API Key',
+ 'password_label' => 'Razorpay API Secret',
+ 'signature_label' => 'Webhook Secret',1-71: Consider adding validation requirements
The configuration should specify any required validation for the API credentials and webhook secret to ensure secure implementation.
Consider adding:
- Validation rules for API credentials format
- SSL/TLS requirements for API communication
- IP whitelist configuration for webhooks
- Recommended security headers
Would you like me to provide a detailed security configuration template?
wp-content/civi-extensions/civirazorpay/civirazorpay.civix.php (2)
61-64: Remove commented out code and add return type declaration.
The path() method contains commented out code and lacks a return type declaration. Clean code practices suggest removing commented out code and maintaining consistent type declarations.
- public static function path($file = NULL) {
+ public static function path(?string $file = NULL): string {
- // return CRM_Core_Resources::singleton()->getPath(self::LONG_NAME, $file);
return __DIR__ . ($file === NULL ? '' : (DIRECTORY_SEPARATOR . $file));
}173-181: Optimize navigation menu fixing to prevent memory leaks.
The recursive menu traversal using array_walk_recursive could lead to memory issues with large navigation structures. Consider using a more memory-efficient approach.
function _civirazorpay_civix_fixNavigationMenu(&$nodes) {
$maxNavID = 1;
- array_walk_recursive($nodes, function($item, $key) use (&$maxNavID) {
- if ($key === 'navID') {
- $maxNavID = max($maxNavID, $item);
- }
- });
+ $maxNavID = self::findMaxNavID($nodes);
_civirazorpay_civix_fixNavigationMenuItems($nodes, $maxNavID, NULL);
}
+ private static function findMaxNavID(array $nodes, int $current = 1): int {
+ foreach ($nodes as $node) {
+ if (isset($node['attributes']['navID'])) {
+ $current = max($current, $node['attributes']['navID']);
+ }
+ if (isset($node['child']) && is_array($node['child'])) {
+ $current = self::findMaxNavID($node['child'], $current);
+ }
+ }
+ return $current;
+ }📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- wp-content/civi-extensions/civirazorpay/LICENSE.txt (1 hunks)
- wp-content/civi-extensions/civirazorpay/README.md (1 hunks)
- wp-content/civi-extensions/civirazorpay/civirazorpay.civix.php (1 hunks)
- wp-content/civi-extensions/civirazorpay/civirazorpay.php (1 hunks)
- wp-content/civi-extensions/civirazorpay/info.xml (1 hunks)
- wp-content/civi-extensions/civirazorpay/managed/PaymentProcessorType.mgd.php (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wp-content/civi-extensions/civirazorpay/info.xml
🧰 Additional context used
🪛 LanguageTool
wp-content/civi-extensions/civirazorpay/LICENSE.txt
[style] ~22-~22: Consider using only “Public” to avoid wordiness.
Context: ...and change the works. By contrast, our General Public Licenses are intended to guarantee your...
(GENERAL_XX)
[style] ~27-~27: Consider using only “Public” to avoid wordiness.
Context: ...e referring to freedom, not price. Our General Public Licenses are designed to make sure that...
(GENERAL_XX)
[style] ~33-~33: Consider using only “Public” to avoid wordiness.
Context: ...hese things. Developers that use our General Public Licenses protect your rights with two s...
(GENERAL_XX)
[uncategorized] ~81-~81: Possible missing comma found.
Context: ...led a "modified version" of the earlier work or a work "based on" the earlier work. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~159-~159: Possible missing comma found.
Context: ...d works that you do not convey, without conditions so long as your license otherwise remai...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~183-~183: ‘To effect’ means ‘to cause’. Did you mean “affected”?
Context: ...res to the extent such circumvention is effected by exercising rights under this License...
(EFFECT_AFFECT)
[style] ~183-~183: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...by exercising rights under this License with respect to the covered work, and you disclaim any ...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[style] ~413-~413: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... the violation by some reasonable means prior to 60 days after the cessation. Moreove...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[style] ~420-~420: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ight holder, and you cure the violation prior to 30 days after your receipt of the notic...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[style] ~431-~431: Consider a shorter alternative to avoid wordiness.
Context: ...are not required to accept this License in order to receive or run a copy of the Program. ...
(IN_ORDER_TO_PREMIUM)
[style] ~433-~433: To make your writing clearer, consider a shorter, more direct phrase.
Context: ...tion of a covered work occurring solely as a consequence of using peer-to-peer transmission to rece...
(AS_A_CONSEQUENCE_OF)
[style] ~475-~475: To make your writing clearer, consider a shorter, more direct phrase.
Context: ...ude claims that would be infringed only as a consequence of further modification of the contributor...
(AS_A_CONSEQUENCE_OF)
[grammar] ~488-~488: ‘an’ may be redundant when used with the uncountable noun ‘permission’.
Context: ...nated, not to enforce a patent (such as an express permission to practice a patent or covenant not to...
(A_UNCOUNTABLE_NOUN)
[grammar] ~501-~501: The verb ‘rely’ requires the preposition ‘on’ (or ‘upon’).
Context: ...e to downstream recipients. "Knowingly relying" means you have actual knowledge that, ...
(RELY_ON)
[misspelling] ~502-~502: Did you mean “you're” (short for ‘you are’)?
Context: ...ledge that, but for the patent license, your conveying the covered work in a country...
(YOUR)
[uncategorized] ~522-~522: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...conveying the work, and under which the third party grants, to any of the parties who would...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~528-~528: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...nt, or that patent license was granted, prior to 28 March 2007. Nothing in this Licen...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[style] ~539-~539: ‘So as to’ expresses purpose and is used in formal texts. Consider using “to”.
Context: ...e. If you cannot convey a covered work so as to satisfy simultaneously your obligations...
(SO_AS_TO)
[typographical] ~635-~635: Conjunctions like ‘and’ should not follow semicolons. Consider using a comma, or removing the conjunction.
Context: ...ectively state the exclusion of warranty; and each file should have at least the "copyrigh...
(CONJUNCTION_AFTER_SEMICOLON)
🪛 Markdownlint
wp-content/civi-extensions/civirazorpay/README.md
2-2: null
Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
8-8: null
Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
8-8: null
Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
12-12: null
Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
12-12: null
Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🔇 Additional comments (2)
wp-content/civi-extensions/civirazorpay/civirazorpay.php (1)
7-32: LGTM! Verify utility functions exist.
The hook implementations are well-documented and follow CiviCRM's best practices. Each hook properly delegates to its corresponding utility function with type-safe declarations.
Let's verify the existence of the utility functions being called:
✅ Verification successful
All utility functions are present and properly implemented
The verification confirms that all three utility functions exist in the civix file:
_civirazorpay_civix_civicrm_config()_civirazorpay_civix_civicrm_install()_civirazorpay_civix_civicrm_enable()
Each utility function is properly implemented with the config function being called as needed by the other functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of utility functions in civix file
# Expected: Should find all three utility function definitions
rg -A 2 "_civirazorpay_civix_civicrm_(config|install|enable)" "wp-content/civi-extensions/civirazorpay/civirazorpay.civix.php"
Length of output: 598
wp-content/civi-extensions/civirazorpay/managed/PaymentProcessorType.mgd.php (1)
63-68: Verify billing mode and payment type configuration
The billing mode is set to 4 (redirect offsite), which is correct for Razorpay's implementation. However, the payment_type is set to 1 (credit card) which might be too restrictive as Razorpay supports multiple payment methods.
Let's verify the supported payment methods in the Razorpay documentation:
✅ Verification successful
Let me check the payment processor type configuration in CiviCRM to understand the standard billing modes and payment types.
Let me check the payment types configuration to complete the verification.
Payment type configuration is appropriate for Razorpay integration
The current configuration with billing_mode = 4 (redirect offsite) and payment_type = 1 (credit card) is actually correct for Razorpay. Here's why:
- The
payment_typein CiviCRM is a binary choice between credit card (1) and direct debit (2), used primarily for form field generation and validation - For offsite processors (billing_mode = 4), the payment_type setting doesn't restrict the actual payment methods available on the payment gateway's side
- The code in CRM/Core/Payment.php confirms this by explicitly skipping credit card validation when billing_mode is 4:
if ($this->_paymentProcessor['billing_mode'] == 4 || $this->_paymentProcessor['payment_type'] != 1) { return FALSE; }
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any payment method configurations or documentation
rg -l "payment.*method|payment.*type" --type php
Length of output: 6801
Script:
#!/bin/bash
# Check PaymentProcessorType documentation and configurations
rg -A 10 "billing_mode.*=|payment_type.*=" wp-content/plugins/civicrm/civicrm/CRM/Financial/BAO/PaymentProcessorType.php wp-content/plugins/civicrm/civicrm/CRM/Financial/DAO/PaymentProcessorType.php wp-content/plugins/civicrm/civicrm/sql/civicrm_data/civicrm_payment_processor_type.sqldata.php
Length of output: 11467
Script:
#!/bin/bash
# Check payment type configurations and documentation
rg -A 5 "payment_type.*=.*[12]" wp-content/plugins/civicrm/civicrm/CRM/Core/Payment.php wp-content/plugins/civicrm/civicrm/sql/civicrm_data/civicrm_payment_processor_type.sqldata.php
# Also check if there are any other payment processors using similar configuration
rg -A 5 "billing_mode.*4.*payment_type.*1" wp-content/civi-extensions/
Length of output: 3420
| 'name' => 'MyProcessor', | ||
| 'entity' => 'payment_processor_type', |
There was a problem hiding this comment.
Update processor name and description to match implementation
The configuration contains generic placeholders that should be updated:
- 'name' is set to 'MyProcessor' but should be 'Razorpay'
- 'description' is set to 'My processor' but should provide meaningful information about the Razorpay payment integration
Apply this diff to fix the inconsistencies:
- 'name' => 'MyProcessor',
+ 'name' => 'Razorpay',
'entity' => 'payment_processor_type',
'params' => [
'version' => 3,
'title' => 'Razorpay',
'name' => 'Razorpay',
- 'description' => 'My processor',
+ 'description' => 'Razorpay Payment Gateway integration for CiviCRM',Also applies to: 40-42
| 'class_name' => 'Payment_MyProcessor', | ||
| // Any urls you might need stored for the user to be redirect to, for example. |
There was a problem hiding this comment.
Update class name to match Razorpay implementation
The class name still contains the generic placeholder 'MyProcessor' instead of the actual Razorpay implementation.
Apply this diff:
- 'class_name' => 'Payment_MyProcessor',
+ 'class_name' => 'Payment_Razorpay',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'class_name' => 'Payment_MyProcessor', | |
| // Any urls you might need stored for the user to be redirect to, for example. | |
| 'class_name' => 'Payment_Razorpay', | |
| // Any urls you might need stored for the user to be redirect to, for example. |
| 'url_site_default' => 'https://example.com', | ||
| 'url_api_default' => 'https://example.com', |
There was a problem hiding this comment.
Replace example.com with actual Razorpay API endpoints
The URLs are currently set to example.com placeholders. These should be updated to the actual Razorpay endpoints.
Apply this diff:
- 'url_site_default' => 'https://example.com',
- 'url_api_default' => 'https://example.com',
+ 'url_site_default' => 'https://api.razorpay.com',
+ 'url_api_default' => 'https://api.razorpay.com/v1',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'url_site_default' => 'https://example.com', | |
| 'url_api_default' => 'https://example.com', | |
| 'url_site_default' => 'https://api.razorpay.com', | |
| 'url_api_default' => 'https://api.razorpay.com/v1', |
| function _civirazorpay_civix_insert_navigation_menu(&$menu, $path, $item) { | ||
| // If we are done going down the path, insert menu | ||
| if (empty($path)) { | ||
| $menu[] = [ | ||
| 'attributes' => array_merge([ | ||
| 'label' => $item['name'] ?? NULL, | ||
| 'active' => 1, | ||
| ], $item), | ||
| ]; | ||
| return TRUE; | ||
| } | ||
| else { | ||
| // Find an recurse into the next level down | ||
| $found = FALSE; | ||
| $path = explode('/', $path); | ||
| $first = array_shift($path); | ||
| foreach ($menu as $key => &$entry) { | ||
| if ($entry['attributes']['name'] == $first) { | ||
| if (!isset($entry['child'])) { | ||
| $entry['child'] = []; | ||
| } | ||
| $found = _civirazorpay_civix_insert_navigation_menu($entry['child'], implode('/', $path), $item); | ||
| } | ||
| } | ||
| return $found; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor navigation menu insertion for better maintainability.
The navigation menu insertion logic is complex and could benefit from being split into smaller, more focused methods. Also, parameter type hints are missing.
- function _civirazorpay_civix_insert_navigation_menu(&$menu, $path, $item) {
+ function _civirazorpay_civix_insert_navigation_menu(array &$menu, string $path, array $item): bool {Consider extracting the recursive logic into a separate method:
private function _insert_menu_item(array &$entry, array $path_parts, array $item): bool {
if (!isset($entry['child'])) {
$entry['child'] = [];
}
return _civirazorpay_civix_insert_navigation_menu(
$entry['child'],
implode('/', $path_parts),
$item
);
}| public static function ts($text, $params = []): string { | ||
| if (!array_key_exists('domain', $params)) { | ||
| $params['domain'] = [self::LONG_NAME, NULL]; | ||
| } | ||
| return ts($text, $params); | ||
| } |
There was a problem hiding this comment.
Add null safety check for ts() function.
The ts() function is called without verifying its existence, which could cause issues if the CiviCRM framework isn't properly initialized.
public static function ts($text, $params = []): string {
+ if (!function_exists('ts')) {
+ return $text;
+ }
if (!array_key_exists('domain', $params)) {
$params['domain'] = [self::LONG_NAME, NULL];
}
return ts($text, $params);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function ts($text, $params = []): string { | |
| if (!array_key_exists('domain', $params)) { | |
| $params['domain'] = [self::LONG_NAME, NULL]; | |
| } | |
| return ts($text, $params); | |
| } | |
| public static function ts($text, $params = []): string { | |
| if (!function_exists('ts')) { | |
| return $text; | |
| } | |
| if (!array_key_exists('domain', $params)) { | |
| $params['domain'] = [self::LONG_NAME, NULL]; | |
| } | |
| return ts($text, $params); | |
| } |
| function _civirazorpay_civix_civicrm_config($config = NULL) { | ||
| static $configured = FALSE; | ||
| if ($configured) { | ||
| return; | ||
| } | ||
| $configured = TRUE; | ||
|
|
||
| $extRoot = __DIR__ . DIRECTORY_SEPARATOR; | ||
| $include_path = $extRoot . PATH_SEPARATOR . get_include_path(); | ||
| set_include_path($include_path); | ||
| // Based on <compatibility>, this does not currently require mixin/polyfill.php. | ||
| } |
There was a problem hiding this comment.
Add error handling for include path modification.
The configuration function modifies the PHP include path without proper error handling.
function _civirazorpay_civix_civicrm_config($config = NULL) {
static $configured = FALSE;
if ($configured) {
return;
}
$configured = TRUE;
$extRoot = __DIR__ . DIRECTORY_SEPARATOR;
$include_path = $extRoot . PATH_SEPARATOR . get_include_path();
- set_include_path($include_path);
+ if (!set_include_path($include_path)) {
+ throw new CRM_Core_Exception('Failed to set include path for extension');
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _civirazorpay_civix_civicrm_config($config = NULL) { | |
| static $configured = FALSE; | |
| if ($configured) { | |
| return; | |
| } | |
| $configured = TRUE; | |
| $extRoot = __DIR__ . DIRECTORY_SEPARATOR; | |
| $include_path = $extRoot . PATH_SEPARATOR . get_include_path(); | |
| set_include_path($include_path); | |
| // Based on <compatibility>, this does not currently require mixin/polyfill.php. | |
| } | |
| function _civirazorpay_civix_civicrm_config($config = NULL) { | |
| static $configured = FALSE; | |
| if ($configured) { | |
| return; | |
| } | |
| $configured = TRUE; | |
| $extRoot = __DIR__ . DIRECTORY_SEPARATOR; | |
| $include_path = $extRoot . PATH_SEPARATOR . get_include_path(); | |
| if (!set_include_path($include_path)) { | |
| throw new CRM_Core_Exception('Failed to set include path for extension'); | |
| } | |
| // Based on <compatibility>, this does not currently require mixin/polyfill.php. | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (5)
wp-content/civi-extensions/civirazorpay/info.xml (2)
21-23: Consider using semantic versioning that reflects alpha status.
For pre-release software in alpha stage, consider using version numbers that clearly indicate the pre-release status.
- <version>1.0.0</version>
+ <version>0.1.0-alpha</version>43-50: Fix XML indentation for consistent formatting.
The indentation in the payment_processors section is inconsistent with the rest of the file.
<payment_processors>
<processor>
- <name>CiviRazorpay</name>
- <title>Razorpay</title>
- <class_name>CiviRazorpay</class_name>
- <billing_mode>form</billing_mode>
- <user_name_label>API Key</user_name_label>
- <password_label>API Secret</password_label>
+ <name>CiviRazorpay</name>
+ <title>Razorpay</title>
+ <class_name>CiviRazorpay</class_name>
+ <billing_mode>form</billing_mode>
+ <user_name_label>API Key</user_name_label>
+ <password_label>API Secret</password_label>
</processor>
</payment_processors>wp-content/civi-extensions/civirazorpay/managed/PaymentProcessorType.mgd.php (1)
47-51: Add validation for required fields and sensitive data.
The configuration handles sensitive credentials (API key, secret) but doesn't specify:
- Which fields are required
- Any validation rules
- Secure handling guidelines for sensitive data
Consider:
- Adding
'is_required' => truefor mandatory fields - Implementing input validation
- Adding comments about secure credential storage
wp-content/civi-extensions/civirazorpay/CRM/Core/Payment/civirazorpay.php (2)
75-76: Check if payment fetch was successful before accessing properties
Accessing properties of $payment without verifying its successful retrieval might cause errors if the fetch failed.
Add a check to ensure $payment is valid:
if ($payment && $payment->status == 'captured') {
// Proceed with processing
} else {
// Handle the error
}86-86: Hardcoded URLs in redirection
Using hardcoded URLs like /civicrm/contribute/success limits flexibility and may cause issues if the application structure changes.
Consider using CiviCRM's URL generation methods:
$successUrl = CRM_Utils_System::url('civicrm/contribute/success', "contribution_id={$contributionID}", true, null, false);
header("Location: {$successUrl}");📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- wp-content/civi-extensions/civirazorpay/CRM/Core/Payment/civirazorpay.php (1 hunks)
- wp-content/civi-extensions/civirazorpay/info.xml (1 hunks)
- wp-content/civi-extensions/civirazorpay/managed/PaymentProcessorType.mgd.php (1 hunks)
- wp-content/civi-extensions/civirazorpay/templates/CRM/Civirazorpay/razorpay_checkout.tpl (1 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/civirazorpay/info.xml (1)
24-26: Verify CiviCRM version compatibility.
Let's verify if version 5.77 is the latest compatible version and if there are any compatibility issues reported.
✅ Verification successful
Version 5.77 appears to be a valid compatibility target
The extension declares compatibility with CiviCRM 5.77, which aligns with current CiviCRM versioning practices:
- The extension is marked as version 1.0.0 and in alpha stage, appropriately indicating its early development status
- CiviCRM 5.77 is a valid version target for extensions
- No compatibility issues or version conflicts were found in the codebase
The version declaration follows best practices by:
- Using semantic versioning for the extension itself
- Explicitly stating CiviCRM core version compatibility
- Including appropriate version metadata in managed files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for CiviCRM version compatibility information in documentation
rg -i "civicr.*version|compatibility" README.md
# Look for version-specific code or compatibility checks
ast-grep --pattern 'CRM_Core_BAO_Domain::version()'
Length of output: 9375
Script:
#!/bin/bash
# Let's try a different approach to check CiviCRM version compatibility
# Check for version references in the extension's info.xml
cat wp-content/civi-extensions/civirazorpay/info.xml
# Look for any version-related files or documentation
fd -t f -e md -e txt version -X cat {}
# Check for any version constraints in composer.json if it exists
fd composer.json -X cat {}
Length of output: 164592
Script:
#!/bin/bash
# Let's check CiviCRM version compatibility information in the codebase
# Look for version references in CiviCRM core
fd -t f -e php -e xml version -x grep -l "CRM_Core_BAO_Domain::version" {} || true
# Check for version compatibility checks in the extension
rg -i "version|compatibility" wp-content/civi-extensions/civirazorpay/
# Check CiviCRM core version requirements in composer.json
fd composer.json -x grep -l "civicrm/civicrm-core" {} || true
Length of output: 6436
wp-content/civi-extensions/civirazorpay/managed/PaymentProcessorType.mgd.php (1)
63-68: Verify billing mode matches Razorpay's integration pattern.
The configuration indicates this is an offsite redirect payment processor (billing_mode = 4) for credit card payments (payment_type = 1). Please verify this matches Razorpay's recommended integration pattern.
✅ Verification successful
The previous search was too specific. Let's broaden it to understand the payment integration pattern by:
- Searching for Razorpay payment-related code with context
- Looking at actual Razorpay integration files
This will help verify if billing_mode=4 (offsite redirect) is correct for Razorpay's integration pattern.
Billing mode 4 (offsite redirect) is correctly configured for Razorpay integration
The code review confirms that billing_mode=4 is the correct configuration for Razorpay integration because:
-
The implementation uses Razorpay's checkout flow where:
- A Razorpay order is created server-side
- User is redirected to Razorpay's checkout page
- After payment, user is redirected back to the IPN handler
-
Key evidence from the codebase:
razorpay_checkout.tplshows redirect handling after paymentdoDirectPayment()creates order and renders checkout pagehandlePaymentNotification()processes the redirect callback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing Razorpay integration patterns in the codebase
rg -i "razorpay.*redirect"
rg -i "razorpay.*billing.mode"
Length of output: 61
Script:
#!/bin/bash
# Let's search for Razorpay integration patterns more broadly
rg -i "razorpay.*payment" -A 5 -B 5
# Also check the main Razorpay class implementation
fd -e php -e js -I razorpay --exec cat {}
Length of output: 16898
| @@ -0,0 +1,24 @@ | |||
| <script src="https://checkout.razorpay.com/v1/checkout.js"></script> | |||
There was a problem hiding this comment.
Add Subresource Integrity (SRI) hash for the external script.
Loading scripts from CDN without SRI validation poses a security risk. Also consider adding error handling for script loading failures.
Apply this diff to improve security:
-<script src="https://checkout.razorpay.com/v1/checkout.js"></script>
+<script src="https://checkout.razorpay.com/v1/checkout.js"
+ integrity="sha384-HASH_VALUE_HERE"
+ crossorigin="anonymous"
+ onerror="handleScriptError()"></script>
+<script>
+function handleScriptError() {
+ console.error('Failed to load Razorpay checkout script');
+ // Show user-friendly error message
+}
+</script>Committable suggestion was skipped due to low confidence.
| var options = { | ||
| "key": "{$razorpayKey}", // Razorpay API Key passed from backend | ||
| "amount": "{$amount}", // Amount in paise | ||
| "currency": "{$currency}", | ||
| "name": "Your Organization Name", | ||
| "description": "Donation", | ||
| "order_id": "{$orderId}", // Razorpay order ID | ||
| "handler": function (response) { | ||
| // Redirect to IPN handler to process payment on server | ||
| window.location.href = "/civicrm/payment/ipn/civirazorpay?payment_id=" + response.razorpay_payment_id + "&order_id=" + response.razorpay_order_id; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Improve payment configuration security and flexibility.
Several issues need attention:
- Hardcoded organization name
- Generic description
- No client-side validation
- Potential XSS vulnerabilities in template variables
Apply these improvements:
var options = {
- "key": "{$razorpayKey}",
- "amount": "{$amount}",
- "currency": "{$currency}",
- "name": "Your Organization Name",
- "description": "Donation",
- "order_id": "{$orderId}",
+ "key": "{$razorpayKey|escape:'javascript'}",
+ "amount": "{$amount|escape:'javascript'}",
+ "currency": "{$currency|escape:'javascript'}",
+ "name": "{$orgName|escape:'javascript'}",
+ "description": "{$paymentDescription|escape:'javascript'}",
+ "order_id": "{$orderId|escape:'javascript'}",
+ "prefill": {
+ "name": "{$contactName|escape:'javascript'}",
+ "email": "{$contactEmail|escape:'javascript'}"
+ },
"handler": function (response) {
- window.location.href = "/civicrm/payment/ipn/civirazorpay?payment_id=" + response.razorpay_payment_id + "&order_id=" + response.razorpay_order_id;
+ // Validate response before redirect
+ if (!response.razorpay_payment_id || !response.razorpay_order_id) {
+ console.error('Invalid payment response');
+ return;
+ }
+ window.location.href = CRM.url('civicrm/payment/ipn/civirazorpay', {
+ payment_id: response.razorpay_payment_id,
+ order_id: response.razorpay_order_id
+ });
}Committable suggestion was skipped due to low confidence.
| var rzp = new Razorpay(options); | ||
|
|
||
| document.getElementById('rzp-button1').onclick = function(e) { | ||
| rzp.open(); | ||
| e.preventDefault(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve payment initialization and error handling.
The current implementation lacks proper error handling and scope management.
Apply this improvement:
-var rzp = new Razorpay(options);
-
-document.getElementById('rzp-button1').onclick = function(e) {
- rzp.open();
- e.preventDefault();
-}
+(function() {
+ let rzp = null;
+
+ function initializeRazorpay() {
+ try {
+ rzp = new Razorpay(options);
+ } catch (error) {
+ console.error('Failed to initialize Razorpay:', error);
+ // Show user-friendly error message
+ return false;
+ }
+ return true;
+ }
+
+ function handlePaymentClick(e) {
+ e.preventDefault();
+ const button = e.target;
+ button.disabled = true;
+
+ try {
+ if (!rzp && !initializeRazorpay()) {
+ button.disabled = false;
+ return;
+ }
+ rzp.open();
+ } catch (error) {
+ console.error('Failed to open payment:', error);
+ button.disabled = false;
+ // Show user-friendly error message
+ }
+ }
+
+ // Clean up on page unload
+ window.addEventListener('unload', function() {
+ if (rzp) {
+ rzp.close();
+ rzp = null;
+ }
+ });
+
+ document.getElementById('razorpay-payment-btn')
+ .addEventListener('click', handlePaymentClick);
+})();Committable suggestion was skipped due to low confidence.
| @@ -0,0 +1,24 @@ | |||
| <script src="https://checkout.razorpay.com/v1/checkout.js"></script> | |||
| <button id="rzp-button1">Pay with Razorpay</button> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve button accessibility and user experience.
The payment button lacks proper accessibility attributes and loading state indication.
Apply these improvements:
-<button id="rzp-button1">Pay with Razorpay</button>
+<button
+ id="razorpay-payment-btn"
+ class="payment-button"
+ aria-label="Pay with Razorpay"
+ data-loading-text="Processing..."
+ disabled="{$disableButton|default:false}">
+ <span class="button-text">Pay with Razorpay</span>
+ <span class="loading-indicator" style="display: none;">
+ <!-- Add loading spinner here -->
+ </span>
+</button>Committable suggestion was skipped due to low confidence.
| <script src="https://checkout.razorpay.com/v1/checkout.js"></script> | ||
| <button id="rzp-button1">Pay with Razorpay</button> | ||
|
|
||
| <script> | ||
| var options = { | ||
| "key": "{$razorpayKey}", // Razorpay API Key passed from backend | ||
| "amount": "{$amount}", // Amount in paise | ||
| "currency": "{$currency}", | ||
| "name": "Your Organization Name", | ||
| "description": "Donation", | ||
| "order_id": "{$orderId}", // Razorpay order ID | ||
| "handler": function (response) { | ||
| // Redirect to IPN handler to process payment on server | ||
| window.location.href = "/civicrm/payment/ipn/civirazorpay?payment_id=" + response.razorpay_payment_id + "&order_id=" + response.razorpay_order_id; | ||
| } | ||
| }; | ||
|
|
||
| var rzp = new Razorpay(options); | ||
|
|
||
| document.getElementById('rzp-button1').onclick = function(e) { | ||
| rzp.open(); | ||
| e.preventDefault(); | ||
| } | ||
| </script> |
There was a problem hiding this comment.
💡 Codebase verification
Integration points are properly implemented but template needs security improvements
The verification reveals that while core integration components are in place, there are some security considerations:
- The IPN handler and payment processor are properly implemented in the codebase
- All required template variables (razorpayKey, amount, currency, orderId) are correctly assigned
- However, template variables are not properly escaped which could lead to XSS vulnerabilities
Recommendations:
- Use
{$razorpayKey|escape:'html'}instead of{$razorpayKey} - Use
{$orderId|escape:'html'}instead of{$orderId} - Use
{$amount|escape:'html'}instead of{$amount} - Use
{$currency|escape:'html'}instead of{$currency}
🔗 Analysis chain
Verify integration points and required template variables.
Let's verify that all required components are in place for this template to function correctly.
Let me verify the IPN handler implementation with another script to ensure proper handling of the payment response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration points for Razorpay checkout
# Check if IPN handler exists
echo "Checking IPN handler..."
rg -l "function civirazorpay" --type php
# Check if template variables are properly assigned
echo "Checking template variable assignment..."
rg -l "assign\(['\"]razorpayKey" --type php
rg -l "assign\(['\"]amount" --type php
rg -l "assign\(['\"]currency" --type php
rg -l "assign\(['\"]orderId" --type php
# Check for potential XSS vulnerabilities in template usage
echo "Checking for unescaped template variables..."
ast-grep --pattern 'assign($_, $x)' --lang php
Length of output: 11841
Script:
#!/bin/bash
# Check IPN handler implementation details
echo "Checking IPN handler implementation..."
rg -A 10 "function civirazorpay" --type php
# Check for payment processing logic
echo "Checking payment processing..."
rg -A 5 "payment_id.*order_id" --type php
# Verify template variable assignments in payment processor
echo "Checking template assignments in payment processor..."
rg -A 3 "assign.*razorpay" wp-content/civi-extensions/civirazorpay/CRM/Core/Payment/civirazorpay.php
Length of output: 2400
| public function doDirectPayment(&$params) { | ||
| // Razorpay API keys | ||
| $apiKey = $this->_paymentProcessor['user_name']; | ||
| $apiSecret = $this->_paymentProcessor['password']; | ||
| $api = new \Razorpay\Api\Api($apiKey, $apiSecret); | ||
|
|
||
| // Create a Razorpay order | ||
| $orderData = [ | ||
| 'amount' => $params['amount'] * 100, // Convert to paise | ||
| 'currency' => 'INR', | ||
| 'receipt' => 'RCPT-' . uniqid(), | ||
| 'payment_capture' => 1 | ||
| ]; | ||
| $order = $api->order->create($orderData); | ||
|
|
||
| // Store Razorpay Order ID as transaction ID in CiviCRM | ||
| $params['trxn_id'] = $order->id; | ||
|
|
||
| // Pass order details to Smarty template | ||
| $smarty = \CRM_Core_Smarty::singleton(); | ||
| $smarty->assign('razorpayKey', $apiKey); | ||
| $smarty->assign('orderId', $order->id); | ||
| $smarty->assign('amount', $params['amount'] * 100); // Amount in paise | ||
| $smarty->assign('currency', 'INR'); | ||
|
|
||
| // Render the Razorpay checkout template | ||
| echo $smarty->fetch('CRM/Civirazorpay/razorpay_checkout.tpl'); | ||
| exit; | ||
| } |
There was a problem hiding this comment.
Secure handling of API credentials
Storing and using API keys directly in the code increases the risk of accidental exposure.
Ensure that API keys are stored securely, such as in environment variables or secured configuration files, and not logged or exposed.
| public function __construct($mode, &$paymentProcessor) { | ||
| parent::__construct($mode, $paymentProcessor); | ||
| $this->_paymentProcessor = $paymentProcessor; | ||
| error_log("CiviRazorpay initialized with payment processor: " . print_r($paymentProcessor, true)); |
There was a problem hiding this comment.
Avoid logging sensitive information
Logging the entire payment processor configuration with print_r($paymentProcessor, true) may expose sensitive data such as API keys and secrets. This is a security risk.
Apply this diff to remove sensitive information from logs:
- error_log("CiviRazorpay initialized with payment processor: " . print_r($paymentProcessor, true));
+ error_log("CiviRazorpay initialized with payment processor.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error_log("CiviRazorpay initialized with payment processor: " . print_r($paymentProcessor, true)); | |
| error_log("CiviRazorpay initialized with payment processor."); |
| ]); | ||
|
|
||
| // Redirect to a thank-you page | ||
| header("Location: /civicrm/contribute/success?contribution_id=" . $contributionID); |
There was a problem hiding this comment.
Sanitize output in header redirection
Directly inserting $contributionID into the header without sanitization can lead to header injection attacks.
Apply this diff to ensure proper sanitization:
- header("Location: /civicrm/contribute/success?contribution_id=" . $contributionID);
+ header("Location: /civicrm/contribute/success?contribution_id=" . urlencode($contributionID));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| header("Location: /civicrm/contribute/success?contribution_id=" . $contributionID); | |
| header("Location: /civicrm/contribute/success?contribution_id=" . urlencode($contributionID)); |
| return E::ts('Payment processor configuration is missing.'); | ||
| } | ||
|
|
||
| if (empty($this->_paymentProcessor['user_name'])) { | ||
| return E::ts('The "Bill To ID" is not set in Administer > CiviContribute > Payment Processor.'); |
There was a problem hiding this comment.
Undefined reference to translation function E::ts
The use of E::ts() for translations may cause an error if the class E is not defined or imported. This can lead to runtime exceptions.
Apply this diff to use the correct translation function:
- return E::ts('Payment processor configuration is missing.');
+ return ts('Payment processor configuration is missing.');
- return E::ts('The "Bill To ID" is not set in Administer > CiviContribute > Payment Processor.');
+ return ts('The "Bill To ID" is not set in Administer > CiviContribute > Payment Processor.');Ensure that the appropriate translation function is used and that necessary classes or namespaces are included.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return E::ts('Payment processor configuration is missing.'); | |
| } | |
| if (empty($this->_paymentProcessor['user_name'])) { | |
| return E::ts('The "Bill To ID" is not set in Administer > CiviContribute > Payment Processor.'); | |
| return ts('Payment processor configuration is missing.'); | |
| } | |
| if (empty($this->_paymentProcessor['user_name'])) { | |
| return ts('The "Bill To ID" is not set in Administer > CiviContribute > Payment Processor.'); |
| $razorpayPaymentId = $_GET['payment_id']; | ||
| $razorpayOrderId = $_GET['order_id']; |
There was a problem hiding this comment.
Sanitize $_GET parameters
Using $_GET['payment_id'] and $_GET['order_id'] without sanitization can introduce security vulnerabilities such as injection attacks.
Apply this diff to sanitize input:
- $razorpayPaymentId = $_GET['payment_id'];
- $razorpayOrderId = $_GET['order_id'];
+ $razorpayPaymentId = filter_input(INPUT_GET, 'payment_id', FILTER_SANITIZE_STRING);
+ $razorpayOrderId = filter_input(INPUT_GET, 'order_id', FILTER_SANITIZE_STRING);Additionally, validate that the parameters meet expected formats.
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Documentation
Metadata