Conversation
📝 WalkthroughWalkthroughThe changes add an LDAP extension availability check to the application. A new error handler method in PanicAttack displays a 500 error if LDAP is missing, and a pre-flight check in index.php invokes this handler before normal initialization. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bootstrap/PanicAttack.php (1)
133-140: Inconsistent brace placement and extra blank line.The opening brace should be on a new line to match the rest of the file (PSR-12 style). Also, there's an extra blank line at the end.
🧹 Proposed style fix
- public function ldap() { + public function ldap() + { $this->title = 'Missing PHP Extension'; $this->code = 500; $this->message = 'The PHP extension "ldap" is required but not loaded. Please install and enable it to run Lychee.'; $this->displaySimpleError(); } - -As per coding guidelines: "Run
vendor/bin/php-cs-fixer fixto apply PHP code style fixes before committing."
| /** | ||
| * Check if the required extensions are loaded and if not, display a nice error page. | ||
| * This is a sanity check to prevent the user from seeing a blank page with an error in the top left corner. | ||
| */ | ||
| if (!extension_loaded('ldap')) { | ||
| $panic_attack = new PanicAttack(); | ||
| $panic_attack->ldap(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for LDAP configuration and references ==="
rg -i 'ldap' -t php --max-count=50 -n
echo ""
echo "=== Checking .env files for LDAP config ==="
find . -name ".env*" -type f -exec grep -H -i ldap {} \; 2>/dev/null || echo "No .env files with LDAP config found"
echo ""
echo "=== Checking if extension_loaded is used conditionally elsewhere ==="
rg "extension_loaded" -t php -n
echo ""
echo "=== Checking config directory structure ==="
fd "config" -t d | head -20
echo ""
echo "=== Searching for PanicAttack class to understand its usage ==="
rg "class PanicAttack" -A 20 -t phpRepository: LycheeOrg/Lychee
Length of output: 50372
🏁 Script executed:
# Quick check: Verify if there's documentation about LDAP being required
rg -i "ldap.*required\|required.*ldap" README.md docs/ 2>/dev/null | head -20 || echo "No documentation about LDAP requirement found"Repository: LycheeOrg/Lychee
Length of output: 42
Make LDAP extension check conditional on configured usage.
The current bootstrap check at public/index.php:25-27 makes the ldap PHP extension mandatory for all Lychee installations. However, LDAP authentication is an optional feature disabled by default (config/ldap.php:52: 'enabled' => false). This inconsistency prevents users who don't use LDAP from running the application if the extension is not installed.
Either move the check to occur only when LDAP authentication is actually enabled in configuration, or remove the hard requirement and let optional features fail gracefully at runtime.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit