[#1922] Added removing of trailing spaces in the installer. #1938
Conversation
WalkthroughAdds trailing-space removal to the installer processing pipeline by importing and invoking a new Strings::removeTrailingSpaces utility in Internal::process(). Introduces the utility method in Strings.php and comprehensive unit tests validating per-line trailing whitespace stripping while preserving line endings and other whitespace. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer as Internal::process
participant File as File::replaceContentAsync
participant Utils as Strings
User->>Installer: Run process()
Installer->>File: replaceContentAsync(callback)
Note over File: For each non-ignored file
File->>File: Existing transforms (e.g., YAML literal-block handling)
File->>Utils: removeTrailingSpaces(content)
Utils-->>File: content (trailing spaces removed per line)
File-->>Installer: Persist transformed content
Installer-->>User: Processing complete
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 phpcs (3.7.2).vortex/installer/src/Prompts/Handlers/Internal.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information .vortex/installer/src/Utils/Strings.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information .vortex/installer/tests/Unit/StringsTest.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vortex/installer/src/Prompts/Handlers/Internal.php (1)
67-73: Skip Markdown-specific gating; no hard-break patterns detectedOur scan found zero occurrences of Markdown lines ending in two or more spaces across all
.md/.markdownfiles, so there’s no current risk of inadvertently breaking hard line breaks. You can simplify the logic by removing the extension check and still guard against repeated empty lines by collapsing a second time after trimming:• File:
.vortex/installer/src/Prompts/Handlers/Internal.php
• Lines: ~67–73if (!$should_ignore) { - $content = File::collapseRepeatedEmptyLines($content); - if (in_array($file->getExtension(), ['yml', 'yaml'], TRUE)) { - $content = Yaml::collapseFirstEmptyLinesInLiteralBlock($content); - } - // Trim all trailing spaces. - $content = Strings::removeTrailingSpaces($content); - // Ensure whitespace-only lines don’t become multiple blanks. - $content = File::collapseRepeatedEmptyLines($content); + // First, collapse repeated blank lines. + $content = File::collapseRepeatedEmptyLines($content); + // Handle YAML literal blocks before trimming. + if (in_array($file->getExtension(), ['yml', 'yaml'], TRUE)) { + $content = Yaml::collapseFirstEmptyLinesInLiteralBlock($content); + } + // Trim trailing spaces everywhere. + $content = Strings::removeTrailingSpaces($content); + // Re-collapse to catch any whitespace-only lines turned blank. + $content = File::collapseRepeatedEmptyLines($content); }This preserves the intended semantics without special-casing Markdown or diff files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
.vortex/installer/tests/Fixtures/install/services_no_valkey/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/docker-compose.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (3)
.vortex/installer/src/Prompts/Handlers/Internal.php(2 hunks).vortex/installer/src/Utils/Strings.php(1 hunks).vortex/installer/tests/Unit/StringsTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-01T08:10:15.903Z
Learnt from: AlexSkrypnyk
PR: drevops/vortex#1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Applied to files:
.vortex/installer/src/Prompts/Handlers/Internal.php
🧬 Code graph analysis (2)
.vortex/installer/src/Prompts/Handlers/Internal.php (1)
.vortex/installer/src/Utils/Strings.php (2)
Strings(7-168)removeTrailingSpaces(164-166)
.vortex/installer/tests/Unit/StringsTest.php (1)
.vortex/installer/src/Utils/Strings.php (2)
Strings(7-168)removeTrailingSpaces(164-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-docs
🔇 Additional comments (3)
.vortex/installer/src/Utils/Strings.php (1)
164-166: LGTM: Correct, safe regex for trimming per-line trailing spaces (incl. CRLF).The implementation is straightforward and matches the PR objective. Good placement within the utility class and solid unit coverage accompanies it.
.vortex/installer/src/Prompts/Handlers/Internal.php (1)
11-11: LGTM: Import looks right and scoped correctly..vortex/installer/tests/Unit/StringsTest.php (1)
447-536: LGTM: Comprehensive coverage for trimming spaces/tabs across LF/CRLF and edge cases.Nice breadth of cases, including mixed endings and last-line-without-newline. This gives high confidence in the utility behavior.
| /** | ||
| * Remove trailing spaces from all lines in a string. | ||
| * | ||
| * @param string $string | ||
| * The input string to process. | ||
| * | ||
| * @return string | ||
| * The string with trailing spaces removed from each line. | ||
| */ | ||
| public static function removeTrailingSpaces(string $string): string { | ||
| return preg_replace('/[ \t]+(?=\r?\n|$)/m', '', $string); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider handling all horizontal whitespace (NBSP, etc.), or clarify scope.
Right now we strip only ASCII space and tab. If authors copy/paste text with non‑breaking spaces or other Unicode horizontal whitespace at EOLs, those will remain. Two options:
- Expand scope: switch to PCRE’s horizontal class with Unicode mode.
- Keep current scope but tighten the docblock language to “spaces and tabs”.
Suggested refactor (expands scope):
- /**
- * Remove trailing spaces from all lines in a string.
- *
- * @param string $string
- * The input string to process.
- *
- * @return string
- * The string with trailing spaces removed from each line.
- */
- public static function removeTrailingSpaces(string $string): string {
- return preg_replace('/[ \t]+(?=\r?\n|$)/m', '', $string);
- }
+ /**
+ * Remove trailing horizontal whitespace from each line.
+ *
+ * Removes spaces, tabs and other Unicode horizontal whitespace before
+ * a line ending or end of string, preserving line endings.
+ */
+ public static function removeTrailingSpaces(string $string): string {
+ return preg_replace('/\h+(?=\r?\n|$)/mu', '', $string);
+ }📝 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.
| /** | |
| * Remove trailing spaces from all lines in a string. | |
| * | |
| * @param string $string | |
| * The input string to process. | |
| * | |
| * @return string | |
| * The string with trailing spaces removed from each line. | |
| */ | |
| public static function removeTrailingSpaces(string $string): string { | |
| return preg_replace('/[ \t]+(?=\r?\n|$)/m', '', $string); | |
| } | |
| /** | |
| * Remove trailing horizontal whitespace from each line. | |
| * | |
| * Removes spaces, tabs and other Unicode horizontal whitespace before | |
| * a line ending or end of string, preserving line endings. | |
| */ | |
| public static function removeTrailingSpaces(string $string): string { | |
| return preg_replace('/\h+(?=\r?\n|$)/mu', '', $string); | |
| } |
🤖 Prompt for AI Agents
.vortex/installer/src/Utils/Strings.php lines 155-166: the current
implementation only strips ASCII spaces and tabs, leaving Unicode horizontal
spaces (e.g. NBSP) at EOL; either narrow the docblock to state "spaces and tabs"
or (preferred) expand the regex to be Unicode-aware by matching
horizontal/separator characters (e.g. use a pattern like matching \t plus \p{Zs}
or \p{Z} with the Unicode 'u' flag) and update the docblock to reflect that it
now removes all horizontal whitespace at line ends; implement the Unicode-aware
regex and add the 'u' modifier, or adjust the docblock text if you decide to
keep the ASCII-only behavior.
| public static function dataProviderRemoveTrailingSpaces(): array { | ||
| return [ | ||
| 'empty_string' => [ | ||
| '', | ||
| '', | ||
| ], | ||
| 'no_trailing_spaces' => [ | ||
| 'hello world', | ||
| 'hello world', | ||
| ], | ||
| 'single_line_with_trailing_spaces' => [ | ||
| 'hello world ', | ||
| 'hello world', | ||
| ], | ||
| 'single_line_with_trailing_tabs' => [ | ||
| "hello world\t\t", | ||
| 'hello world', | ||
| ], | ||
| 'single_line_with_mixed_trailing_whitespace' => [ | ||
| "hello world \t \t", | ||
| 'hello world', | ||
| ], | ||
| 'multiline_with_trailing_spaces' => [ | ||
| "line one \nline two \nline three", | ||
| "line one\nline two\nline three", | ||
| ], | ||
| 'multiline_with_trailing_tabs' => [ | ||
| "line one\t\t\nline two\t\nline three", | ||
| "line one\nline two\nline three", | ||
| ], | ||
| 'multiline_with_mixed_trailing_whitespace' => [ | ||
| "line one \t \nline two \t\nline three \t\t ", | ||
| "line one\nline two\nline three", | ||
| ], | ||
| 'empty_lines_with_trailing_spaces' => [ | ||
| "line one\n \nline three", | ||
| "line one\n\nline three", | ||
| ], | ||
| 'empty_lines_with_trailing_tabs' => [ | ||
| "line one\n\t\t\nline three", | ||
| "line one\n\nline three", | ||
| ], | ||
| 'only_trailing_whitespace_lines' => [ | ||
| " \n\t\t\n \t ", | ||
| "\n\n", | ||
| ], | ||
| 'preserve_leading_whitespace' => [ | ||
| " indented line \n\tindented with tab\t", | ||
| " indented line\n\tindented with tab", | ||
| ], | ||
| 'preserve_internal_whitespace' => [ | ||
| "hello world \ninternal spaces\t", | ||
| "hello world\ninternal spaces", | ||
| ], | ||
| 'windows_line_endings' => [ | ||
| "line one \r\nline two\t\r\nline three", | ||
| "line one\r\nline two\r\nline three", | ||
| ], | ||
| 'mixed_line_endings' => [ | ||
| "line one \nline two\t\r\nline three ", | ||
| "line one\nline two\r\nline three", | ||
| ], | ||
| 'single_space_at_end' => [ | ||
| 'hello world ', | ||
| 'hello world', | ||
| ], | ||
| 'single_tab_at_end' => [ | ||
| "hello world\t", | ||
| 'hello world', | ||
| ], | ||
| 'multiple_consecutive_trailing_spaces' => [ | ||
| 'hello world ', | ||
| 'hello world', | ||
| ], | ||
| 'multiple_consecutive_trailing_tabs' => [ | ||
| "hello world\t\t\t\t", | ||
| 'hello world', | ||
| ], | ||
| 'no_line_ending_with_trailing_spaces' => [ | ||
| 'single line with spaces ', | ||
| 'single line with spaces', | ||
| ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: Add a Unicode NBSP case if you broaden trimming to all horizontal whitespace.
If you adopt the suggested \h + u change in Strings::removeTrailingSpaces(), add a provider case to lock in behavior for non-breaking spaces:
'multiple_consecutive_trailing_tabs' => [
"hello world\t\t\t\t",
'hello world',
],
+ 'unicode_nbsp_at_eol' => [
+ "hello\u{00A0}",
+ 'hello',
+ ],I can follow up with an integration test in the installer pipeline to assert Markdown hard breaks are preserved if we gate trimming by extension. Want me to sketch that?
📝 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 dataProviderRemoveTrailingSpaces(): array { | |
| return [ | |
| 'empty_string' => [ | |
| '', | |
| '', | |
| ], | |
| 'no_trailing_spaces' => [ | |
| 'hello world', | |
| 'hello world', | |
| ], | |
| 'single_line_with_trailing_spaces' => [ | |
| 'hello world ', | |
| 'hello world', | |
| ], | |
| 'single_line_with_trailing_tabs' => [ | |
| "hello world\t\t", | |
| 'hello world', | |
| ], | |
| 'single_line_with_mixed_trailing_whitespace' => [ | |
| "hello world \t \t", | |
| 'hello world', | |
| ], | |
| 'multiline_with_trailing_spaces' => [ | |
| "line one \nline two \nline three", | |
| "line one\nline two\nline three", | |
| ], | |
| 'multiline_with_trailing_tabs' => [ | |
| "line one\t\t\nline two\t\nline three", | |
| "line one\nline two\nline three", | |
| ], | |
| 'multiline_with_mixed_trailing_whitespace' => [ | |
| "line one \t \nline two \t\nline three \t\t ", | |
| "line one\nline two\nline three", | |
| ], | |
| 'empty_lines_with_trailing_spaces' => [ | |
| "line one\n \nline three", | |
| "line one\n\nline three", | |
| ], | |
| 'empty_lines_with_trailing_tabs' => [ | |
| "line one\n\t\t\nline three", | |
| "line one\n\nline three", | |
| ], | |
| 'only_trailing_whitespace_lines' => [ | |
| " \n\t\t\n \t ", | |
| "\n\n", | |
| ], | |
| 'preserve_leading_whitespace' => [ | |
| " indented line \n\tindented with tab\t", | |
| " indented line\n\tindented with tab", | |
| ], | |
| 'preserve_internal_whitespace' => [ | |
| "hello world \ninternal spaces\t", | |
| "hello world\ninternal spaces", | |
| ], | |
| 'windows_line_endings' => [ | |
| "line one \r\nline two\t\r\nline three", | |
| "line one\r\nline two\r\nline three", | |
| ], | |
| 'mixed_line_endings' => [ | |
| "line one \nline two\t\r\nline three ", | |
| "line one\nline two\r\nline three", | |
| ], | |
| 'single_space_at_end' => [ | |
| 'hello world ', | |
| 'hello world', | |
| ], | |
| 'single_tab_at_end' => [ | |
| "hello world\t", | |
| 'hello world', | |
| ], | |
| 'multiple_consecutive_trailing_spaces' => [ | |
| 'hello world ', | |
| 'hello world', | |
| ], | |
| 'multiple_consecutive_trailing_tabs' => [ | |
| "hello world\t\t\t\t", | |
| 'hello world', | |
| ], | |
| 'no_line_ending_with_trailing_spaces' => [ | |
| 'single line with spaces ', | |
| 'single line with spaces', | |
| ], | |
| ]; | |
| } | |
| public static function dataProviderRemoveTrailingSpaces(): array { | |
| return [ | |
| // ... other cases ... | |
| 'multiple_consecutive_trailing_tabs' => [ | |
| "hello world\t\t\t\t", | |
| 'hello world', | |
| ], | |
| 'unicode_nbsp_at_eol' => [ | |
| "hello\u{00A0}", | |
| 'hello', | |
| ], | |
| 'no_line_ending_with_trailing_spaces' => [ | |
| 'single line with spaces ', | |
| 'single line with spaces', | |
| ], | |
| // ... any following cases ... | |
| ]; | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1938 +/- ##
========================================
Coverage 76.21% 76.22%
========================================
Files 87 87
Lines 5381 5383 +2
Branches 35 35
========================================
+ Hits 4101 4103 +2
Misses 1280 1280 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #1922
Summary by CodeRabbit
Bug Fixes
Style
Tests