Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable limit for the number of records exported, adding a new setting 'ExportMaxRecords' to the configuration. The review feedback suggests improving the robustness of the implementation by casting the configuration value to an integer and treating all non-positive values as an 'unlimited' export option, along with updating the configuration documentation to reflect this behavior.
| $maxExportRecords = GetConfigSetting("ExportMaxRecords", 10000, CFGLEVEL_GLOBAL); | ||
| if ($maxExportRecords == 0) { | ||
| // No limit - export all matching records | ||
| $maxExportRecords = PHP_INT_MAX; | ||
| } |
There was a problem hiding this comment.
For better robustness, it's good practice to explicitly cast the configuration value to an integer. Also, handling non-positive values (i.e., <= 0) instead of just == 0 for the unlimited case would prevent unexpected behavior if a user configures a negative number, which would currently result in no records being exported.
$maxExportRecords = (int) GetConfigSetting("ExportMaxRecords", 10000, CFGLEVEL_GLOBAL);
if ($maxExportRecords <= 0) {
// No limit - export all matching records
$maxExportRecords = PHP_INT_MAX;
}|
|
||
| // --- Default Export options | ||
| $CFG['ExportUseTodayYesterday'] = 0; // Same as ViewUseTodayYesterday. By default export normal dates | ||
| $CFG['ExportMaxRecords'] = 10000; // Maximum number of records to export. 0 means no limit (export all matching records) |
There was a problem hiding this comment.
To align with the improved logic of treating any non-positive value as 'no limit', it would be clearer to update this comment to reflect that both 0 and negative values result in unlimited exports. This helps prevent user confusion during configuration.
$CFG['ExportMaxRecords'] = 10000; // Maximum number of records to export. 0 or a negative value means no limit (export all matching records)
|
@cubic-dev-ai review |
@alorbach I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
2 participants
This pull request introduces a new configuration setting to control the maximum number of records exported in the
src/export.phpfile. The changes ensure that exports are limited based on this setting, improving performance and preventing excessive data handling.Configuration Update:
src/include/config.sample.php: Added a new configuration setting,ExportMaxRecords, with a default value of 10,000. This setting allows administrators to define the maximum number of records to export, with0indicating no limit.Export Logic Enhancement:
src/export.php: Introduced a variable$maxExportRecordsto fetch theExportMaxRecordsconfiguration value. If set to0, it defaults toPHP_INT_MAXto allow unlimited exports. This ensures flexibility in export limits.src/export.php: Updated the export loop condition to use$maxExportRecordsinstead of the previousCurrentViewEntriesPerPage, effectively enforcing the export limit.