Add aws ses extension for bounce email functionality #381
Conversation
WalkthroughThe pull request introduces significant enhancements to the AWS SES extension for CiviCRM. Key changes include the addition of multiple classes for managing upgrades, handling email bounces and complaints, processing incoming requests, and parsing source addresses. Utility functions for resource management are also included. The extension's settings and menu structure have been defined in new files, and documentation has been added. Additionally, the license has been updated to include the GNU Affero General Public License version 3.0. Changes
Suggested reviewers
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: 41
🧹 Outside diff range and nitpick comments (17)
wp-content/civi-extensions/aws-ses-master/xml/Menu/ses.xml (1)
1-1: Consider specifying the XML encoding.While the XML declaration is present, it's a good practice to explicitly specify the encoding. This ensures proper interpretation of the file across different systems.
Consider updating the XML declaration to include the encoding:
-<?xml version="1.0"?> +<?xml version="1.0" encoding="UTF-8"?>wp-content/civi-extensions/aws-ses-master/mixin/[email protected] (2)
16-20: Event listener setup looks good. Minor suggestion for improvement.The event listener setup is well-implemented:
- Correctly uses CiviCRM's dispatcher to listen for the 'hook_civicrm_xmlMenu' event.
- Anonymous function as a listener provides flexibility.
- Proper use of closure to capture $mixInfo.
Suggestion for improved readability:
Consider extracting the anonymous function to a named function within the outer function scope. This can make the code more self-documenting and easier to understand at a glance.Example:
$handleXmlMenu = function ($e) use ($mixInfo) { // ... (rest of the function body) }; Civi::dispatcher()->addListener('hook_civicrm_xmlMenu', $handleXmlMenu);This change is optional but could enhance code clarity.
25-28: Good approach to XML file processing. Some improvements suggested.The XML file processing logic is straightforward and effective:
- Uses glob() to find XML files in the correct directory.
- Iterates through found files and adds them to the event object.
However, there are opportunities for improvement:
- Error Handling: Add checks for glob() failure and empty results.
- File Validation: Ensure files are readable before adding them.
Suggested improvements:
$pattern = $mixInfo->getPath('xml/Menu/*.xml'); $files = glob($pattern); if ($files === false) { // Log error or throw exception return; } foreach ($files as $file) { if (is_readable($file)) { $e->files[] = $file; } else { // Log warning about unreadable file } }These changes will make the code more robust and prevent potential issues with unreadable files.
wp-content/civi-extensions/aws-ses-master/Civi/SES/SourceAddress.php (4)
5-8: Enhance class documentationThe current comment above the class doesn't provide much useful information about the class's purpose or functionality. Consider replacing it with a more descriptive PHPDoc comment that explains the class's role in parsing source addresses for AWS SES.
Here's a suggested improvement:
/** * Class SourceAddress * * Provides functionality to parse email addresses used in AWS SES for CiviCRM. * This class is responsible for extracting components from specially formatted * email addresses used in the bounce and complaint handling process. */ class SourceAddress {
10-10: Add parameter and return type declarationsTo improve code clarity and take advantage of PHP's type system, consider adding parameter and return type declarations to the
parsemethod.Here's the suggested change:
public static function parse(string $address): array|false
15-16: Improve regex pattern readabilityThe regex pattern is complex and could benefit from explanation. Consider breaking it down into named components and adding comments to explain each part.
Here's a suggested improvement:
// Local part of the email $localPart = preg_quote($mailSettings['localpart']); // Action code (b|c|e|o|r|u) $actionCode = '(b|c|e|o|r|u)'; // Separator defined in Civi settings $separator = preg_quote(\Civi::settings()->get('verpSeparator')); // Job ID, Event Queue ID, and Hash $jobId = '(\d+)'; $queueId = '(\d+)'; $hash = '([0-9a-f]{16})'; // Domain part of the email $domain = preg_quote($mailSettings['domain']); $regex = "/^{$localPart}{$actionCode}{$separator}{$jobId}{$separator}{$queueId}{$separator}{$hash}@{$domain}$/";
25-25: Use array destructuring instead of list()The
list()function is considered outdated in modern PHP. Consider using array destructuring instead.Here's the suggested change:
[$match, $action, $job, $queue, $hash] = $matches;wp-content/civi-extensions/aws-ses-master/mixin/[email protected] (1)
22-29: Solid logic with room for minor improvement!The function body contains good practices such as early return and proper checks before modifying the settings folders. However, to improve readability, consider extracting the condition for adding the settings directory into a separate variable with a descriptive name.
Here's a suggested refactor for improved clarity:
$settingsDir = $mixInfo->getPath('settings'); -if (!in_array($settingsDir, $e->settingsFolders) && is_dir($settingsDir)) { +$shouldAddSettingsDir = !in_array($settingsDir, $e->settingsFolders) && is_dir($settingsDir); +if ($shouldAddSettingsDir) { $e->settingsFolders[] = $settingsDir; }wp-content/civi-extensions/aws-ses-master/info.xml (1)
11-16: Consider refining the URLs section for better user guidance.While the URLs provided are functional, there are a couple of points to consider:
- The main extension page and documentation URLs are identical. It might be beneficial to have separate, more specific URLs for each if possible.
- The support URL is generic. Consider providing a more specific support page or resource for this extension.
These changes could enhance the user experience by providing more targeted information and support resources.
wp-content/civi-extensions/aws-ses-master/aws_ses.php (3)
17-33: Properly implemented install and enable hooks, but consider DRYing it up!The
aws_ses_civicrm_installandaws_ses_civicrm_enablefunctions are correctly implemented and well-documented. They follow CiviCRM extension best practices by delegating to civix functions.However, these functions are nearly identical in structure. To adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring them into a single helper function:
function _aws_ses_implement_hook($hook_name) { $function_name = "_aws_ses_civix_civicrm_{$hook_name}"; if (function_exists($function_name)) { $function_name(); } } function aws_ses_civicrm_install() { _aws_ses_implement_hook('install'); } function aws_ses_civicrm_enable() { _aws_ses_implement_hook('enable'); }This refactoring would make the code more maintainable and reduce duplication.
35-44: Clean up or clarify the commented-out preProcess hookThe commented-out
aws_ses_civicrm_preProcessfunction adds unnecessary clutter to the codebase. If this hook is planned for future implementation, consider replacing it with a TODO comment explaining the intended functionality and when it might be implemented. If there are no immediate plans to use this hook, it's best to remove the commented-out code entirely.Example of a more informative TODO:
// TODO: Implement hook_civicrm_preProcess when form preprocessing for AWS SES is required. // This may be necessary for [specific functionality] in the future. // See: https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_preProcessRemember, clean code is easier to maintain and understand!
46-61: Excellent navigation menu implementation with room for minor improvementThe
aws_ses_civicrm_navigationMenufunction is well-implemented and follows CiviCRM best practices. It correctly adds the AWS SES Settings menu item and uses translation functions appropriately.To improve readability, consider extracting the menu item configuration into a separate variable:
function aws_ses_civicrm_navigationMenu(&$menu) { $awsSesMenuItem = [ 'label' => E::ts('AWS SES Settings'), 'name' => 'mailing_aws_ses_settings', 'url' => 'civicrm/admin/settings/aws-ses', 'permission' => 'access CiviMail', 'operator' => 'OR', 'separator' => 0, ]; _aws_ses_civix_insert_navigation_menu($menu, 'Administer/CiviMail', $awsSesMenuItem); _aws_ses_civix_navigationMenu($menu); }This small change makes the function easier to read and maintain, especially if more menu items need to be added in the future.
wp-content/civi-extensions/aws-ses-master/Civi/SES/Bounce.php (3)
5-5: Add class-level docblock forBounceclassTo enhance code readability and adhere to coding standards, consider adding a docblock for the
Bounceclass that describes its purpose and usage.
14-29: Add documentation for theprocess()methodThe
process()method lacks a docblock. Adding a docblock will improve code documentation and maintainability by clearly explaining the method's functionality and parameters.
35-35: Fix typo in commentThere's a typo in the comment: "let process only permanent bounces and add an extra check for bouce recipients" should be "let's process only permanent bounces and add an extra check for bounce recipients."
wp-content/civi-extensions/aws-ses-master/mixin/polyfill.php (1)
8-8: Correct typo in commentThe word "persnickity" in the comment at line 8 appears to be misspelled. The correct spelling is "persnickety."
wp-content/civi-extensions/aws-ses-master/Civi/SES/BounceType.php (1)
9-76: Standardize Bounce Type Keys for ConsistencyThe keys used in the
$typesarray mix spaces, colons, and different casing styles (e.g.,'SES undetermined','SES permanent: general',"SES complaint"). For better readability and consistency, consider adopting a uniform naming convention such assnake_caseorPascalCasewithout special characters or spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- wp-content/civi-extensions/aws-ses-master/CRM/AwsSes/Upgrader.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/CRM/AwsSes/Upgrader/Base.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/Civi/SES/Bounce.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/Civi/SES/BounceType.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/Civi/SES/Complaint.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/Civi/SES/Page/Listen.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/Civi/SES/SourceAddress.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/LICENSE.txt (1 hunks)
- wp-content/civi-extensions/aws-ses-master/README.md (1 hunks)
- wp-content/civi-extensions/aws-ses-master/aws_ses.civix.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/aws_ses.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/info.xml (1 hunks)
- wp-content/civi-extensions/aws-ses-master/mixin/[email protected] (1 hunks)
- wp-content/civi-extensions/aws-ses-master/mixin/polyfill.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/mixin/[email protected] (1 hunks)
- wp-content/civi-extensions/aws-ses-master/settings/ses.setting.php (1 hunks)
- wp-content/civi-extensions/aws-ses-master/xml/Menu/ses.xml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wp-content/civi-extensions/aws-ses-master/README.md
🧰 Additional context used
🪛 LanguageTool
wp-content/civi-extensions/aws-ses-master/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] ~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)
🔇 Additional comments (16)
wp-content/civi-extensions/aws-ses-master/settings/ses.setting.php (1)
1-13: Well-structured configuration file. Nicely done!The configuration file is well-organized and follows common PHP practices for defining settings. The 'aws_ses_secret' setting is properly defined with all necessary attributes.
wp-content/civi-extensions/aws-ses-master/xml/Menu/ses.xml (2)
2-2: LGTM: Root menu element structure is correct.The root
element is properly structured and contains the expected child elements for CiviCRM's menu XML format.Also applies to: 15-15
9-14: LGTM: AWS SES settings menu item is well-structured.The menu item for AWS SES settings is correctly configured:
- The path and title are clear and appropriate.
- It uses the generic admin form, which is suitable for settings pages.
- Access is properly restricted to users with CiviCRM administration privileges.
wp-content/civi-extensions/aws-ses-master/mixin/[email protected] (2)
1-14: Well-structured file header and function signature. Approved!The file header and function signature demonstrate good coding practices:
- Clear and informative docblock explaining the mixin's purpose and parameters.
- Use of type hinting in the function signature for improved code clarity.
- Modern PHP syntax with an anonymous function return, suitable for mixin definition.
Keep up the good work!
21-23: Excellent use of early return pattern. Approved!The mixin activity check is well-implemented:
- Efficiently checks if the mixin is active before proceeding.
- Uses the early return pattern to simplify code flow and avoid unnecessary nesting.
- Demonstrates awareness of potential mixin state changes.
This is a great example of writing clean, efficient code. Keep it up!
wp-content/civi-extensions/aws-ses-master/Civi/SES/SourceAddress.php (1)
3-3: Namespace declaration looks good!The namespace
Civi\SESis correctly declared and follows PSR-4 autoloading standards, aligning with the file path structure.wp-content/civi-extensions/aws-ses-master/mixin/[email protected] (2)
1-14: Excellent documentation and clean function signature!The file header and function signature are well-structured and clearly documented. The use of a returned anonymous function is an appropriate pattern for creating mixins in this context.
20-30: Proper use of event dispatcher and closure!The event listener is correctly set up using Civi::dispatcher()->addListener(). The use of a closure to capture $mixInfo is an effective way to provide the necessary context to the listener function.
wp-content/civi-extensions/aws-ses-master/info.xml (4)
1-5: LGTM! Extension metadata looks good.The extension key, file name, name, and description are consistent and accurately reflect the purpose of the AWS SES mail processor extension for CiviCRM.
6-10: License and maintainer information are properly defined.The AGPL-3.0 license is clearly stated, and the maintainer's details are provided. This information is crucial for users and contributors of the extension.
28-32: Civix configuration looks good.The civix section is well-defined with appropriate namespace, Angular module name, and format version. This configuration will help ensure proper integration with CiviCRM.
33-38: Mixins and upgrader configuration are properly set up.The inclusion of menu-xml and setting-php mixins will facilitate menu and settings management for the extension. The defined upgrader class (CRM_AwsSes_Upgrader) is crucial for handling future updates and database schema changes. This setup follows CiviCRM best practices for extension development.
wp-content/civi-extensions/aws-ses-master/aws_ses.php (2)
1-6: Good start with proper setup and coding standards awareness!The file begins with the correct PHP opening tag, includes necessary files, and shows attention to coding standards with the phpcs comments. The use of an alias for the extension utility is a nice touch for improved readability.
8-15: Well-implemented config hook with proper documentation!The
aws_ses_civicrm_configfunction is correctly implemented and documented. It follows CiviCRM extension best practices by delegating to the civix function. The included documentation link is helpful for future maintainers.wp-content/civi-extensions/aws-ses-master/aws_ses.civix.php (1)
179-207: Ensure Unique Navigation Menu IDsThe functions
_aws_ses_civix_fixNavigationMenu()and_aws_ses_civix_fixNavigationMenuItems()assign navigation IDs to menu items. It's important to ensure that these IDs are unique to prevent conflicts in the navigation structure.Review the logic for ID assignment and test the menu generation to confirm that all IDs are unique and the menu hierarchy is maintained correctly.
wp-content/civi-extensions/aws-ses-master/CRM/AwsSes/Upgrader/Base.php (1)
49-54: Verify the correct instantiation ininstance()method.In the
instance()method, the classCRM_AwsSes_Upgraderis instantiated instead ofCRM_AwsSes_Upgrader_Base:self::$instance = new CRM_AwsSes_Upgrader( 'aws-ses', E::path() );Ensure that
CRM_AwsSes_Upgraderis correctly defined and extendsCRM_AwsSes_Upgrader_Base. IfCRM_AwsSes_Upgraderis not properly set up, this could lead to unexpected behaviors or errors.To confirm this, you can search for the
CRM_AwsSes_Upgraderclass in the codebase:
| 'aws_ses_secret' => [ | ||
| 'name' => 'aws_ses_secret', | ||
| 'type' => 'String', | ||
| 'html_type' => 'text', | ||
| 'title' => 'SNS secret', | ||
| 'description' => "'secret' query parameter added to the AWS SNS endpoint", | ||
| 'is_domain' => 1, | ||
| 'is_contact' => 0, | ||
| 'settings_pages' => ['aws-ses' => ['weight' => 10]], | ||
| ], |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Recommend Enhancements for aws_ses_secret Configuration
-
Security Improvement: It's advisable to manage
aws_ses_secretusing environment variables or a secure secret management system instead of storing it in configuration files. This reduces the risk of exposing sensitive information. -
Description Enhancement: Update the description to provide clearer context. For example:
- "Secret key used to verify incoming SNS notifications. This should match the 'secret' query parameter added to the AWS SNS endpoint URL. For enhanced security, consider setting this via the AWS_SES_SECRET environment variable."
🔗 Analysis chain
Consider security implications and enhance the description
While the structure is good, there are two points to consider:
-
Storing secrets in configuration files can be risky. Consider using environment variables or a secure secret management system instead.
-
The description could be more informative. It currently says:
"'secret' query parameter added to the AWS SNS endpoint"Consider expanding this to provide more context, such as:
"Secret key used to verify incoming SNS notifications. This should match the 'secret' query parameter added to the AWS SNS endpoint URL."
To ensure we're not exposing sensitive information, let's check if this secret is used or referenced elsewhere in the codebase:
Consider refactoring to use environment variables for the secret. You could modify the code to:
'aws_ses_secret' => [
'name' => 'aws_ses_secret',
'type' => 'String',
'html_type' => 'text',
'title' => 'SNS secret',
'description' => "Secret key used to verify incoming SNS notifications. This should match the 'secret' query parameter added to the AWS SNS endpoint URL. For security, consider setting this via the AWS_SES_SECRET environment variable.",
'default' => getenv('AWS_SES_SECRET') ?: '',
'is_domain' => 1,
'is_contact' => 0,
'settings_pages' => ['aws-ses' => ['weight' => 10]],
],This approach allows for more secure secret management while maintaining compatibility with the current structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to aws_ses_secret
rg --type php 'aws_ses_secret'
Length of output: 594
| <item> | ||
| <path>civicrm/aws-ses/listen</path> | ||
| <page_callback>\Civi\SES\Page\Listen</page_callback> | ||
| <title>AWS SES endpoint</title> | ||
| <access_callback>1</access_callback> | ||
| </item> |
There was a problem hiding this comment.
Review the access control for the AWS SES endpoint.
The current configuration allows unrestricted access to the AWS SES endpoint. This could potentially expose sensitive information or functionality.
Consider implementing more restrictive access control. For example:
- <access_callback>1</access_callback>
+ <access_callback>CRM_Core_Permission::check</access_callback>
+ <access_arguments>access CiviCRM,access CiviMail</access_arguments>This change would restrict access to users with both 'access CiviCRM' and 'access CiviMail' permissions. Adjust the permissions as necessary for your use case.
📝 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.
| <item> | |
| <path>civicrm/aws-ses/listen</path> | |
| <page_callback>\Civi\SES\Page\Listen</page_callback> | |
| <title>AWS SES endpoint</title> | |
| <access_callback>1</access_callback> | |
| </item> | |
| <item> | |
| <path>civicrm/aws-ses/listen</path> | |
| <page_callback>\Civi\SES\Page\Listen</page_callback> | |
| <title>AWS SES endpoint</title> | |
| <access_callback>CRM_Core_Permission::check</access_callback> | |
| <access_arguments>access CiviCRM,access CiviMail</access_arguments> | |
| </item> |
| */ | ||
| class SourceAddress { | ||
|
|
||
| public static function parse($address) { |
There was a problem hiding this comment.
Add input validation for $address parameter
The method currently doesn't validate the input parameter. Consider adding a check to ensure $address is a non-empty string before proceeding with the parsing logic.
Here's a suggested addition at the beginning of the method:
if (empty($address) || !is_string($address)) {
return false;
}| public static function parse($address) { | ||
| $mailSettings = \Civi\Api4\MailSettings::get(FALSE) | ||
| ->addWhere('is_default', '=', TRUE) | ||
| ->execute()->first(); | ||
|
|
||
| $vs = preg_quote(\Civi::settings()->get('verpSeparator')); | ||
| $regex = '/^' . preg_quote($mailSettings['localpart']) . '(b|c|e|o|r|u)' . $vs . '(\d+)' . $vs . '(\d+)' . $vs . '([0-9a-f]{16})@' . preg_quote($mailSettings['domain']) . '$/'; | ||
| $matches = array(); | ||
|
|
||
| preg_match($regex, $address, $matches); | ||
|
|
||
| if (!$matches) { | ||
| return FALSE; | ||
| } | ||
|
|
||
| list($match, $action, $job, $queue, $hash) = $matches; | ||
|
|
||
| $params = array( | ||
| 'job_id' => $job, | ||
| 'event_queue_id' => $queue, | ||
| 'hash' => $hash, | ||
| ); | ||
|
|
||
| return $params; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down the parse method
The parse method is currently responsible for multiple tasks: retrieving settings, constructing the regex, and parsing the address. This violates the Single Responsibility Principle. Consider breaking these tasks into separate private methods to improve readability and maintainability.
Here's a suggested refactoring:
public static function parse(string $address): array|false
{
if (empty($address) || !is_string($address)) {
return false;
}
$regex = self::buildRegex();
$matches = [];
if (!preg_match($regex, $address, $matches)) {
return false;
}
return self::extractComponents($matches);
}
private static function buildRegex(): string
{
$mailSettings = self::getMailSettings();
// ... build and return the regex
}
private static function getMailSettings(): array
{
return \Civi\Api4\MailSettings::get(FALSE)
->addWhere('is_default', '=', TRUE)
->execute()->first();
}
private static function extractComponents(array $matches): array
{
[, , $job, $queue, $hash] = $matches;
return [
'job_id' => $job,
'event_queue_id' => $queue,
'hash' => $hash,
];
}| <releaseDate>2023-10-26</releaseDate> | ||
| <version>1.1</version> | ||
| <develStage>alpha</develStage> |
There was a problem hiding this comment.
Clarify version number and development stage consistency.
The extension is marked as version 1.1, which typically indicates it's not the first release. However, the development stage is set to "alpha". This combination might be confusing to users. Consider the following options:
- If this is truly an alpha release, adjust the version number to reflect early development (e.g., 0.1 or 0.1.1).
- If the extension is more mature, consider updating the development stage to "beta" or "stable" as appropriate.
Ensuring consistency between version number and development stage will provide clearer expectations for users of the extension.
| public function onInstall() { | ||
| $files = glob($this->extensionDir . '/sql/*_install.sql'); | ||
| if (is_array($files)) { | ||
| foreach ($files as $file) { | ||
| CRM_Utils_File::sourceSQLFile(CIVICRM_DSN, $file); | ||
| } | ||
| } | ||
| $files = glob($this->extensionDir . '/sql/*_install.mysql.tpl'); | ||
| if (is_array($files)) { | ||
| foreach ($files as $file) { | ||
| $this->executeSqlTemplate($file); | ||
| } | ||
| } | ||
| $files = glob($this->extensionDir . '/xml/*_install.xml'); | ||
| if (is_array($files)) { | ||
| foreach ($files as $file) { | ||
| $this->executeCustomDataFileByAbsPath($file); | ||
| } | ||
| } | ||
| if (is_callable([$this, 'install'])) { | ||
| $this->install(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate code in onInstall method.
The onInstall method includes repeated blocks for processing SQL files, SQL templates, and XML files. This duplication can make the code harder to maintain and extend. Consider abstracting these blocks into separate helper methods to simplify the code and adhere to the DRY (Don't Repeat Yourself) principle.
Apply this refactoring to extract the duplicated code:
private function processFiles($pattern, $callback) {
$files = glob($this->extensionDir . $pattern);
if (is_array($files)) {
foreach ($files as $file) {
call_user_func($callback, $file);
}
}
}
public function onInstall() {
$this->processFiles('/sql/*_install.sql', function($file) {
CRM_Utils_File::sourceSQLFile(CIVICRM_DSN, $file);
});
$this->processFiles('/sql/*_install.mysql.tpl', function($file) {
$this->executeSqlTemplate($file);
});
$this->processFiles('/xml/*_install.xml', function($file) {
$this->executeCustomDataFileByAbsPath($file);
});
if (is_callable([$this, 'install'])) {
$this->install();
}
}| public function onUninstall() { | ||
| $files = glob($this->extensionDir . '/sql/*_uninstall.mysql.tpl'); | ||
| if (is_array($files)) { | ||
| foreach ($files as $file) { | ||
| $this->executeSqlTemplate($file); | ||
| } | ||
| } | ||
| if (is_callable([$this, 'uninstall'])) { | ||
| $this->uninstall(); | ||
| } | ||
| $files = glob($this->extensionDir . '/sql/*_uninstall.sql'); | ||
| if (is_array($files)) { | ||
| foreach ($files as $file) { | ||
| CRM_Utils_File::sourceSQLFile(CIVICRM_DSN, $file); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate code in onUninstall method.
Similar to onInstall, the onUninstall method has repeated code blocks for processing files. Refactoring this code will improve maintainability and readability.
Update the onUninstall method using the same helper function:
public function onUninstall() {
$this->processFiles('/sql/*_uninstall.mysql.tpl', function($file) {
$this->executeSqlTemplate($file);
});
if (is_callable([$this, 'uninstall'])) {
$this->uninstall();
}
$this->processFiles('/sql/*_uninstall.sql', function($file) {
CRM_Utils_File::sourceSQLFile(CIVICRM_DSN, $file);
});
}| public static function _queueAdapter() { | ||
| $instance = self::instance(); | ||
| $args = func_get_args(); | ||
| $instance->ctx = array_shift($args); | ||
| $instance->queue = $instance->ctx->queue; | ||
| $method = array_shift($args); | ||
| return call_user_func_array([$instance, $method], $args); | ||
| } |
There was a problem hiding this comment.
Avoid modifying shared state in _queueAdapter method.
The _queueAdapter method modifies instance properties $ctx and $queue on the singleton instance, which can lead to issues in concurrent or asynchronous environments due to shared mutable state.
Consider refactoring to pass $ctx and $queue as method parameters instead of setting them as instance properties. This approach enhances the method's reusability and thread safety.
| private function getCurrentRevisionDeprecated() { | ||
| $key = $this->extensionName . ':version'; | ||
| if ($revision = \Civi::settings()->get($key)) { | ||
| $this->revisionStorageIsDeprecated = TRUE; | ||
| } | ||
| return $revision; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deprecate usage of getCurrentRevisionDeprecated() method.
The method getCurrentRevisionDeprecated() is used to handle legacy revision storage:
private function getCurrentRevisionDeprecated() {
// ...
}Consider migrating any remaining dependencies away from this deprecated method and removing it from the codebase to simplify maintenance and reduce confusion.
| } | ||
| } |
There was a problem hiding this comment.
Handle default case in onUpgrade method.
The default case in the switch statement of the onUpgrade method is empty:
default:Consider adding a logging statement or throwing an exception to handle unexpected values of $op. This will improve the robustness of your code by ensuring that all cases are accounted for.
Example:
default:
throw new CRM_Core_Exception("Unknown operation '$op' in onUpgrade method.");
Add aws ses extension for bounce email functionality
Summary by CodeRabbit
New Features
Documentation
README.mdto provide detailed information on the AWS SES integration.Chores