Issue#242165 Feat: Development of Prefix and Postfix to input group #388
Conversation
Release 1.4.6
WalkthroughThe changes introduce new language constants for currency formatting options in the TJFields component and add a migration script that updates three database tables by adding a new column, Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Administrator
participant Migration as TjHouseKeepingTjVendorsColumn
participant DB as Database
Admin->>Migration: Call migrate()
loop For each table (#__tj_city, #__tj_region, #__tj_country)
Migration->>DB: Check if com_tjvendors column exists
alt Column does not exist
Migration->>DB: ALTER TABLE to add com_tjvendors
DB-->>Migration: Success or Failure
else Column exists
Note over Migration: No action needed
end
end
Migration-->>Admin: Return status and message
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
administrator/houseKeeping/1.4.6/tjVendorsColumn.php (2)
29-29: Fix incorrect return type annotation.The method actually returns an array, not void.
- * @return void + * @return array
41-43: Inconsistent query building pattern.The code inconsistently uses
$db->getQuery(true)followed by string assignment. Either use the query builder properly or use direct string assignment consistently.- $query = $db->getQuery(true); - $query = "SHOW COLUMNS FROM `#__tj_city`"; + $query = "SHOW COLUMNS FROM `#__tj_city`";Apply similar changes for the other two table queries.
Also applies to: 62-64, 83-85
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
administrator/models/forms/types/forms/number.xmlis excluded by!**/*.xml
📒 Files selected for processing (2)
admin_language/en-GB/en-GB.com_tjfields.ini(1 hunks)administrator/houseKeeping/1.4.6/tjVendorsColumn.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.php`: "Review the PHP code for Joomla coding standards compliance, ensuring: - Code follows Joomla's coding standards, validated using PHPCS with the Joomla standard. - Adher...
**/*.php: "Review the PHP code for Joomla coding standards compliance, ensuring:
- Code follows Joomla's coding standards, validated using PHPCS with the Joomla standard.
- Adherence to Joomla’s MVC structure for extensions.
- Proper use of Joomla’s built-in functions (e.g., Input, Factory).
- SQL queries use Joomla’s database API to prevent SQL injection.
- Code is well-documented, with clear function and class descriptions."
administrator/houseKeeping/1.4.6/tjVendorsColumn.php
🔇 Additional comments (1)
admin_language/en-GB/en-GB.com_tjfields.ini (1)
209-214: LGTM! Currency formatting constants are well-structured.The new language constants follow proper Joomla naming conventions and provide clear, descriptive text for the currency formatting functionality. The examples in the currency symbol description (₹, $, €) are helpful for users.
| $query = $db->getQuery(true); | ||
| $query = "SHOW COLUMNS FROM `#__tj_region`"; | ||
| $db->setQuery($query); | ||
| $columns = $db->loadobjectlist(); |
There was a problem hiding this comment.
Fix inconsistent method calls.
loadobjectlist() should be loadObjectList() (proper camelCase) to match the first usage at line 43.
- $columns = $db->loadobjectlist();
+ $columns = $db->loadObjectList();Apply the same fix on line 85:
- $columns = $db->loadobjectlist();
+ $columns = $db->loadObjectList();Also applies to: 85-85
🤖 Prompt for AI Agents
In administrator/houseKeeping/1.4.6/tjVendorsColumn.php at lines 64 and 85, the
method loadobjectlist() is incorrectly cased and should be changed to
loadObjectList() to maintain consistent camelCase usage as seen at line 43.
Update both lines to call loadObjectList() instead of loadobjectlist().
| if (!$db->execute()) | ||
| { | ||
| $result['status'] = false; | ||
| $result['message'] = $db->getErrorMsg(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated getErrorMsg() method.
getErrorMsg() is deprecated. Use modern exception handling or getErrorNum() with proper error checking.
- $result['message'] = $db->getErrorMsg();
+ $result['message'] = 'Database error: ' . $db->getErrorNum() . ' - ' . $db->getErrorMsg();Alternatively, consider letting database exceptions bubble up and handling them in the catch block.
Also applies to: 76-76, 97-97
🤖 Prompt for AI Agents
In administrator/houseKeeping/1.4.6/tjVendorsColumn.php at lines 55, 76, and 97,
the code uses the deprecated getErrorMsg() method to retrieve database error
messages. Replace these calls by implementing proper exception handling around
the database operations or use getErrorNum() combined with conditional checks to
detect errors. If exceptions are enabled, allow them to propagate and catch them
in a try-catch block to handle errors more robustly and replace the error
message assignment accordingly.
|
|
||
| try | ||
| { | ||
| $db = JFactory::getDbo(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated JFactory with Factory.
JFactory::getDbo() is deprecated in Joomla 4+. Use the modern Factory class instead.
+use Joomla\CMS\Factory;
+
- $db = JFactory::getDbo();
+ $db = Factory::getDbo();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In administrator/houseKeeping/1.4.6/tjVendorsColumn.php at line 39, replace the
deprecated call to JFactory::getDbo() with the modern Factory class method.
Import the Factory class if not already imported, then use Factory::getDbo() to
obtain the database object, ensuring compatibility with Joomla 4+ standards.
| try | ||
| { | ||
| $db = JFactory::getDbo(); | ||
| $query = $db->getQuery(true); | ||
| $query = "SHOW COLUMNS FROM `#__tj_city`"; | ||
| $db->setQuery($query); | ||
| $columns = $db->loadAssoclist(); | ||
|
|
||
| $columns = array_column($columns, "Field"); | ||
|
|
||
| if (!in_array('com_tjvendors', $columns)) | ||
| { | ||
| $query = "ALTER TABLE `#__tj_city` ADD COLUMN `com_tjvendors` tinyint(1) NOT NULL DEFAULT '1'"; | ||
| $db->setQuery($query); | ||
|
|
||
| if (!$db->execute()) | ||
| { | ||
| $result['status'] = false; | ||
| $result['message'] = $db->getErrorMsg(); | ||
|
|
||
| return $result; | ||
| } | ||
| } | ||
|
|
||
| $query = $db->getQuery(true); | ||
| $query = "SHOW COLUMNS FROM `#__tj_region`"; | ||
| $db->setQuery($query); | ||
| $columns = $db->loadobjectlist(); | ||
|
|
||
| $columns = array_column($columns, "Field"); | ||
|
|
||
| if (!in_array('com_tjvendors', $columns)) | ||
| { | ||
| $query = "ALTER TABLE `#__tj_region` ADD COLUMN `com_tjvendors` tinyint(1) NOT NULL DEFAULT '1'"; | ||
| $db->setQuery($query); | ||
|
|
||
| if (!$db->execute()) | ||
| { | ||
| $result['status'] = false; | ||
| $result['message'] = $db->getErrorMsg(); | ||
|
|
||
| return $result; | ||
| } | ||
| } | ||
|
|
||
| $query = $db->getQuery(true); | ||
| $query = "SHOW COLUMNS FROM `#__tj_country`"; | ||
| $db->setQuery($query); | ||
| $columns = $db->loadobjectlist(); | ||
|
|
||
| $columns = array_column($columns, "Field"); | ||
|
|
||
| if (!in_array('com_tjvendors', $columns)) | ||
| { | ||
| $query = "ALTER TABLE `#__tj_country` ADD COLUMN `com_tjvendors` tinyint(1) NOT NULL DEFAULT '1'"; | ||
| $db->setQuery($query); | ||
|
|
||
| if (!$db->execute()) | ||
| { | ||
| $result['status'] = false; | ||
| $result['message'] = $db->getErrorMsg(); | ||
|
|
||
| return $result; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication with helper method.
The same logic is repeated three times for different tables. Extract this into a reusable method to follow DRY principles.
+ /**
+ * Add com_tjvendors column to a table if it doesn't exist
+ *
+ * @param string $tableName The table name
+ * @param object $db Database object
+ *
+ * @return array Result array with status and message
+ *
+ * @since 1.4.6
+ */
+ private function addColumnToTable($tableName, $db)
+ {
+ $query = "SHOW COLUMNS FROM `#__tj_{$tableName}`";
+ $db->setQuery($query);
+ $columns = $db->loadObjectList();
+ $columns = array_column($columns, "Field");
+
+ if (!in_array('com_tjvendors', $columns))
+ {
+ $query = "ALTER TABLE `#__tj_{$tableName}` ADD COLUMN `com_tjvendors` tinyint(1) NOT NULL DEFAULT '1'";
+ $db->setQuery($query);
+
+ if (!$db->execute())
+ {
+ return array(
+ 'status' => false,
+ 'message' => 'Database error: ' . $db->getErrorNum() . ' - ' . $db->getErrorMsg()
+ );
+ }
+ }
+
+ return array('status' => true);
+ }Then simplify the migrate method:
public function migrate()
{
- $result = array();
-
try
{
$db = Factory::getDbo();
- $query = $db->getQuery(true);
- $query = "SHOW COLUMNS FROM `#__tj_city`";
- $db->setQuery($query);
- $columns = $db->loadAssoclist();
-
- $columns = array_column($columns, "Field");
-
- if (!in_array('com_tjvendors', $columns))
- {
- $query = "ALTER TABLE `#__tj_city` ADD COLUMN `com_tjvendors` tinyint(1) NOT NULL DEFAULT '1'";
- $db->setQuery($query);
-
- if (!$db->execute())
- {
- $result['status'] = false;
- $result['message'] = $db->getErrorMsg();
-
- return $result;
- }
- }
-
- // Similar code blocks for region and country...
+
+ $tables = ['city', 'region', 'country'];
+
+ foreach ($tables as $table)
+ {
+ $result = $this->addColumnToTable($table, $db);
+
+ if (!$result['status'])
+ {
+ return $result;
+ }
+ }
- $result['status'] = true;
- $result['message'] = "Migration successful";
+ return array(
+ 'status' => true,
+ 'message' => 'Migration successful'
+ );
}
catch (Exception $e)
{
- $result['err_code'] = '';
- $result['status'] = false;
- $result['message'] = $e->getMessage();
+ return array(
+ 'err_code' => '',
+ 'status' => false,
+ 'message' => $e->getMessage()
+ );
}
-
- return $result;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In administrator/houseKeeping/1.4.6/tjVendorsColumn.php between lines 37 and
101, the code repeats the same logic for checking and adding the 'com_tjvendors'
column on three different tables. To fix this, create a helper method that
accepts a table name, checks if the column exists, and adds it if missing. Then
replace the repeated blocks with calls to this helper method for each table,
reducing duplication and improving maintainability.
2 participants
Summary by CodeRabbit