Update .htaccess rewrite and error handling logic

[d0ubIeU]

Enhanced the adjustRewriteBaseHtaccess method to also configure ErrorDocument 404 in addition to RewriteBase. This ensures proper URL routing and error handling by directing 404 errors to the application's error handler.

Summary by CodeRabbit

• Setup & Configuration
  • Improved .htaccess handling during setup: better detection and validation of rewrite directives, enhanced routing for 404 error pages to point to the correct index route, and more robust update/writing behavior to reduce misconfiguration risk.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The adjustRewriteBaseHtaccess method in EnvironmentConfigurator now updates both RewriteBase and ErrorDocument 404 in .htaccess, validates directives before modification, writes the file, and returns a boolean indicating success; documented exceptions are preserved.

Changes

Cohort / File(s)	Summary
Htaccess Configuration Enhancement phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php	Extended adjustRewriteBaseHtaccess to handle RewriteBase and ErrorDocument 404 directives, validate before modifying, update ErrorDocument 404 to use /index.php?action=404, write changes to .htaccess, return boolean success, and expanded docblock with return and exception details

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Single-file change focused on directive parsing, validation, and file I/O
• Verify regex/parse logic for RewriteBase and ErrorDocument 404 handles edge cases (leading/trailing slashes, existing query paths)
• Confirm atomicity of file write and proper error propagation when writing fails

Poem

🐇 I nibbled lines of htaccess bright,

Rewrote the base and set 404 right,
Validated paths with careful care,
Saved the file and hopped away in flair.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the main change: updating .htaccess rewrite and error handling logic to handle both RewriteBase and ErrorDocument 404 directives.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php (2)

97-102: Avoid reparsing .htaccess and simplify RewriteBase update

Here you already have the RewriteBase directive token from $htaccess->search(...), but then call getRewriteBase(), which reparses the file just to obtain the current argument and then immediately overwrite it.

You can simplify and make this more robust by:

• Dropping the extra parse and removeArgument(...) call.
• Directly setting the new base from getServerPath().

For example:

-        $rewriteBase = $htaccess->search('RewriteBase', TOKEN_DIRECTIVE);
-        if ($rewriteBase) {
-            $rewriteBase->removeArgument($this->getRewriteBase());
-            $rewriteBase->setArguments((array) $this->getServerPath());
-        }
+        $rewriteBase = $htaccess->search('RewriteBase', TOKEN_DIRECTIVE);
+        if ($rewriteBase) {
+            $rewriteBase->setArguments([$this->getServerPath()]);
+        }

This avoids redundant IO and keeps all RewriteBase handling within the already‑parsed $htaccess structure.

---

71-79: Docblock looks good; align with formatter/CS tooling

The expanded docblock clearly reflects the new behavior and return type. However, CI reports mago format --dry-run differences for this file; please run mago format locally so the docblock and surrounding code match the project’s formatting rules (likely whitespace/blank‑line details).

#!/bin/bash
# From repo root
mago format
git diff

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f960c93 and af9d3b9.

📒 Files selected for processing (1)

• phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.php

📄 CodeRabbit inference engine (AGENTS.md)

Use PSR-12 coding standards for PHP code

Files:

• phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php

**/*.{php,ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{php,ts,tsx,js,jsx}: Use semicolons at the end of each statement
Use single quotes for strings
Follow best practices for localization, such as using placeholders for dynamic content and avoiding hard-coded strings

Files:

• phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php

🪛 GitHub Actions: phpMyFAQ Build

phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php

[error] 69-69: mago format --dry-run reported formatting differences. Differences detected between original and formatted file. Command 'mago format --dry-run' failed with exit code 1. Run 'mago format' to auto-fix.

🔇 Additional comments (1)

phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php (1)

104-109: [rewritten comment]
[classification tag]

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php (1)

97-102: RewriteBase adjustment works but re-parses .htaccess and can be simplified

Functionally this block is fine, but a couple of points are worth tightening up:

• You already have the $rewriteBase token from $htaccess->search(), but you re-parse the same .htaccess file inside getRewriteBase(). This is redundant work and another potential failure point.
• Since Directive::setArguments() appends new arguments instead of clearing the existing list, relying on removeArgument($this->getRewriteBase()) only works if the current value exactly matches what getRewriteBase() returns and if there’s only a single argument.(github.com)

You can avoid both issues by clearing the current arguments directly on the existing token and then adding the new base, for example:

-        $rewriteBase = $htaccess->search('RewriteBase', TOKEN_DIRECTIVE);
-        if ($rewriteBase) {
-            $rewriteBase->removeArgument($this->getRewriteBase());
-            $rewriteBase->setArguments((array) $this->getServerPath());
-        }
+        $rewriteBase = $htaccess->search('RewriteBase', TOKEN_DIRECTIVE);
+        if ($rewriteBase) {
+            foreach ($rewriteBase->getArguments() as $argument) {
+                $rewriteBase->removeArgument((string) $argument);
+            }
+
+            $rewriteBase->addArgument((string) $this->getServerPath());
+        }

This keeps everything on the already-parsed token and guarantees a single, correct RewriteBase argument.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between af9d3b9 and 21c8ae5.

📒 Files selected for processing (1)

• phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.php

📄 CodeRabbit inference engine (AGENTS.md)

Use PSR-12 coding standards for PHP code

Files:

• phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php

**/*.{php,ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{php,ts,tsx,js,jsx}: Use semicolons at the end of each statement
Use single quotes for strings
Follow best practices for localization, such as using placeholders for dynamic content and avoiding hard-coded strings

Files:

• phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php

⏰ 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). (4)

• GitHub Check: phpMyFAQ 8.6 Test on ubuntu-latest
• GitHub Check: phpMyFAQ 8.4 Test on ubuntu-latest
• GitHub Check: phpMyFAQ 8.3 Test on ubuntu-latest
• GitHub Check: phpMyFAQ 8.5 Test on ubuntu-latest

🔇 Additional comments (2)

phpmyfaq/src/phpMyFAQ/Setup/EnvironmentConfigurator.php (2)

71-79: Docblock update is clear and consistent with behavior

The expanded description (RewriteBase + ErrorDocument 404, boolean return, and exception conditions) accurately reflects what the method does and reads well. No changes needed.

---

104-109: ErrorDocument 404 handling requires verification of htaccess-parser library behavior

The code at lines 104–109 calls search('ErrorDocument 404', TOKEN_DIRECTIVE) and then setArguments(), but several concerns warrant verification:

1. Search pattern plausibility: The search uses 'ErrorDocument 404' (including the status code) whereas the working RewriteBase search uses only the directive name. If the underlying search() method matches against only the directive name (not including arguments), this pattern would fail to match.

2. Argument handling asymmetry: The RewriteBase code explicitly calls removeArgument() before setArguments(), but the ErrorDocument 404 code does not. This pattern inconsistency suggests potential issues with how setArguments() handles existing arguments.

3. Missing test coverage: The test suite (EnvironmentConfiguratorTest) only verifies that RewriteBase is updated correctly; it contains no test that confirms ErrorDocument 404 is actually modified in the .htaccess file. This gap makes it impossible to verify the code works as intended.

To resolve these concerns, verify the actual behavior of Tivie\HtaccessParser v0.4.0—specifically:

• How HtaccessContainer::search() matches patterns (does it use only the directive name or include arguments?)
• How Directive::setArguments() handles existing arguments (does it replace or append?)

If the concerns are confirmed, the fix suggested in the review comment (iterating through tokens, finding the 404 directive, and carefully managing arguments) is sound.