Open
Conversation
Reviewer's GuideReplaces legacy CSV import/export infrastructure (including deprecated profile CSV import and SpoonCSV-based reader/writer usage) with PhpSpreadsheet-native CSV handling, centralizes CSV reading logic in ForkCMS\Utility\Csv\Reader, and upgrades phpoffice/phpspreadsheet to v5.4 while keeping user-specific CSV settings behavior intact. Sequence diagram for profiles CSV import using new ReadersequenceDiagram
actor AdminUser
participant Browser
participant ImportAction as Backend_Modules_Profiles_Actions_Import
participant Auth as Backend_Core_Engine_Authentication
participant User as Backend_Core_Engine_User
participant CsvReader as ForkCMS_Utility_Csv_Reader
participant ProfilesModel as Backend_Modules_Profiles_Engine_Model
AdminUser ->> Browser: Upload CSV and submit import form
Browser ->> ImportAction: HTTP POST /profiles/import
ImportAction ->> ImportAction: validateForm()
ImportAction ->> Auth: getUser()
Auth -->> ImportAction: Backend_Core_Engine_User
ImportAction ->> CsvReader: findColumnIndexes(path, [email,display_name,password], user)
CsvReader -->> ImportAction: columnIndexes
ImportAction ->> ImportAction: validate required columns
ImportAction ->> Auth: getUser()
Auth -->> ImportAction: Backend_Core_Engine_User
ImportAction ->> CsvReader: convertFileToArray(path, mapping, user)
CsvReader -->> ImportAction: csvData array
ImportAction ->> ProfilesModel: importFromArray(csvData, groupId, overwriteExisting)
ProfilesModel -->> ImportAction: statistics
ImportAction ->> Browser: HTTP redirect to profiles index with report
Browser -->> AdminUser: Shows import result message
Class diagram for updated CSV utilities and profiles import flowclassDiagram
class ForkCMS_Utility_Csv_Reader {
+findColumnIndexes(path string, columns array, user Backend_Core_Engine_User) array
+convertFileToArray(path string, mapping array, user Backend_Core_Engine_User) array
+convertRowIntoMappedArray(row PhpOffice_PhpSpreadsheet_Worksheet_Row, mapping array) array
-getUserOptions(user Backend_Core_Engine_User) array
-getReader(options array) PhpOffice_PhpSpreadsheet_Reader_Csv
}
class ForkCMS_Utility_Csv_Writer {
<<readonly>>
-charset string
+__construct(charset string)
+getResponse(spreadsheet PhpOffice_PhpSpreadsheet_Spreadsheet, filename string, options array) Symfony_Component_HttpFoundation_StreamedResponse
+getResponseForUser(spreadsheet PhpOffice_PhpSpreadsheet_Spreadsheet, filename string, user Backend_Core_Engine_User) Symfony_Component_HttpFoundation_StreamedResponse
-getDefaultOptions() array
-getUserOptions(user Backend_Core_Engine_User) array
-getWriter(spreadsheet PhpOffice_PhpSpreadsheet_Spreadsheet, options array) PhpOffice_PhpSpreadsheet_Writer_Csv
-getStreamedResponse(writer PhpOffice_PhpSpreadsheet_Writer_Csv, filename string) Symfony_Component_HttpFoundation_StreamedResponse
}
class Backend_Modules_Profiles_Actions_Import {
-form Backend_Core_Engine_Form
+execute() void
-validateForm() void
}
class Backend_Modules_Profiles_Engine_Model {
+importFromArray(data array, groupId int, overwriteExisting bool) array
}
class Backend_Core_Engine_Authentication {
+getUser() Backend_Core_Engine_User
}
class Backend_Core_Engine_User {
+getSetting(name string) mixed
}
class PhpOffice_PhpSpreadsheet_Spreadsheet
class PhpOffice_PhpSpreadsheet_Writer_Csv
class PhpOffice_PhpSpreadsheet_Reader_Csv
class PhpOffice_PhpSpreadsheet_Worksheet_Row
class Symfony_Component_HttpFoundation_StreamedResponse
Backend_Modules_Profiles_Actions_Import --> ForkCMS_Utility_Csv_Reader : uses
Backend_Modules_Profiles_Actions_Import --> Backend_Modules_Profiles_Engine_Model : imports_profiles_via
Backend_Modules_Profiles_Actions_Import --> Backend_Core_Engine_Authentication : gets_current_user
ForkCMS_Utility_Csv_Reader --> Backend_Core_Engine_User : reads_csv_settings
ForkCMS_Utility_Csv_Reader --> PhpOffice_PhpSpreadsheet_Reader_Csv : configures_reader
ForkCMS_Utility_Csv_Writer --> Backend_Core_Engine_User : reads_csv_settings
ForkCMS_Utility_Csv_Writer --> PhpOffice_PhpSpreadsheet_Spreadsheet : writes_from
ForkCMS_Utility_Csv_Writer --> PhpOffice_PhpSpreadsheet_Writer_Csv : configures_writer
ForkCMS_Utility_Csv_Writer --> Symfony_Component_HttpFoundation_StreamedResponse : returns
Backend_Core_Engine_Authentication --> Backend_Core_Engine_User : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Reader::getUserOptions assumes
csv_split_characterandcsv_line_endingare always set on the user; consider adding sane defaults or guards so the new CSV reader behavior doesn’t break when these settings are missing or misconfigured.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Reader::getUserOptions assumes `csv_split_character` and `csv_line_ending` are always set on the user; consider adding sane defaults or guards so the new CSV reader behavior doesn’t break when these settings are missing or misconfigured.
## Individual Comments
### Comment 1
<location path="src/ForkCMS/Utility/Csv/Reader.php" line_range="13-26" />
<code_context>
+ private function getUserOptions(User $user): array
{
- $reader = IOFactory::createReader('Csv');
+ $options['Delimiter'] = $user->getSetting('csv_split_character');
+
+ $lineEnding = $user->getSetting('csv_line_ending');
+ if ($lineEnding === '\n') {
+ $options['LineEnding'] = "\n";
+ }
+ if ($lineEnding === '\r\n') {
+ $options['LineEnding'] = "\r\n";
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guard against null/empty CSV settings so defaults from PhpSpreadsheet still apply when user settings are not configured.
As written, `csv_split_character` / `csv_line_ending` will always populate `$options`, even when unset or empty, overriding the Csv reader’s defaults with null/empty values. Please only set `Delimiter` and `LineEnding` when the retrieved setting is non-empty so the reader can fall back to its built-in defaults otherwise.
```suggestion
private function getUserOptions(User $user): array
{
$options = [];
$delimiter = $user->getSetting('csv_split_character');
if (!empty($delimiter)) {
$options['Delimiter'] = $delimiter;
}
$lineEnding = $user->getSetting('csv_line_ending');
if (!empty($lineEnding)) {
if ($lineEnding === '\n') {
$options['LineEnding'] = "\n";
} elseif ($lineEnding === '\r\n') {
$options['LineEnding'] = "\r\n";
}
}
return $options;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This pull request removes the deprecated SpoonCSV dependency by replacing it with direct usage of PhpOffice/PhpSpreadsheet for CSV reading and writing operations. The changes modernize the CSV handling infrastructure by removing the legacy Backend\Core\Engine\Csv class that extended SpoonFileCSV and consolidating CSV operations in the ForkCMS\Utility\Csv namespace.
Changes:
- Removed deprecated Backend\Core\Engine\Csv class and BackendProfilesModel::importCsv() method
- Refactored Writer and Reader classes to use PhpSpreadsheet directly with user-specific CSV options
- Updated Import action to use the refactored Reader service for both column detection and file conversion
- Updated phpoffice/phpspreadsheet version constraint in composer.json
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| composer.json | Updates phpoffice/phpspreadsheet version constraint |
| src/ForkCMS/Utility/Csv/Writer.php | Converts to readonly class, removes unused imports, replaces IOFactory with direct instantiation, simplifies method call syntax |
| src/ForkCMS/Utility/Csv/Reader.php | Adds getUserOptions and getReader methods, adds new convertFileToArray method, updates findColumnIndexes to accept User parameter |
| src/Backend/Modules/Profiles/Engine/Model.php | Removes deprecated importCsv method |
| src/Backend/Modules/Profiles/Actions/Import.php | Updates to use Reader service for file conversion, removes local convertFileToArray method, corrects documentation |
| src/Backend/Modules/Profiles/Actions/ExportTemplate.php | Updates documentation comment |
| src/Backend/Core/Engine/Csv.php | Completely removes deprecated class file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /** | ||
| * This is the add-action, it will display a form to add a new profile. | ||
| * This is the import-action, it will display to import a CSV file with profiles to create. |
There was a problem hiding this comment.
The documentation comment has a grammatical error. "display to import" should be "display a form to import" for proper English grammar.
Suggested change
| * This is the import-action, it will display to import a CSV file with profiles to create. | |
| * This is the import-action, it will display a form to import a CSV file with profiles to create. |
| readonly class Writer | ||
| { | ||
| public function __construct(private readonly string $charset) | ||
| public function __construct(private string $charset) |
There was a problem hiding this comment.
In a readonly class, all instance properties must be declared as readonly. The property $charset should be declared as 'private readonly string $charset' instead of 'private string $charset'. This is a PHP language requirement for readonly classes.
Suggested change
| public function __construct(private string $charset) | |
| public function __construct(private readonly string $charset) |
Comment on lines
+13
to
+25
| private function getUserOptions(User $user): array | ||
| { | ||
| $reader = IOFactory::createReader('Csv'); | ||
| $options['Delimiter'] = $user->getSetting('csv_split_character'); | ||
|
|
||
| $lineEnding = $user->getSetting('csv_line_ending'); | ||
| if ($lineEnding === '\n') { | ||
| $options['LineEnding'] = "\n"; | ||
| } | ||
| if ($lineEnding === '\r\n') { | ||
| $options['LineEnding'] = "\r\n"; | ||
| } | ||
|
|
||
| return $options; |
There was a problem hiding this comment.
The $options array is used without being initialized. If neither line ending condition is met (lines 18-23), the method would return an undefined variable. Initialize $options as an empty array at the beginning of the method: $options = [];
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See https://next-app.activecollab.com/108877/projects/5029?modal=Task-243463-62
Summary by Sourcery
Remove legacy CSV import code and centralize CSV handling on PhpSpreadsheet-based utilities.
Enhancements:
Build: