Fix cache persistence of views #3940
Conversation
ildyria
added
the
ignore-for-release
📝 WalkthroughWalkthroughA migration file was updated to safely handle missing foreign keys using try-catch error handling, and the Docker entrypoint script was modified to include additional Laravel cache clearing commands in web mode to optimize initialization. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks✅ Passed checks (1 passed)
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: 0
🧹 Nitpick comments (1)
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php (1)
31-37: Consider logging the exception for consistency with similar patterns.The defensive try-catch approach is appropriate for handling missing foreign keys during migration. However, based on learnings from version bump migrations, similar error-handling patterns log warnings before proceeding. While silently ignoring is safe here (since the FK absence is the expected fallback state), adding a log statement would improve debuggability and maintain consistency with established patterns in the codebase.
🔎 Optional: Add logging for consistency
try { Schema::table('size_variants', function (Blueprint $table) { $table->dropForeign(['photo_id']); }); } catch (\Exception $e) { - // If the foreign key does not exist, we can safely ignore the error + // If the foreign key does not exist, we can safely ignore the error + \Log::warning('Foreign key on size_variants.photo_id does not exist, skipping drop.'); }Based on learnings from similar migration patterns in the Lychee codebase.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.phpdocker/scripts/entrypoint.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php
🧠 Learnings (7)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3774
File: database/migrations/2025_10_28_214205_bump_version060904.php:30-41
Timestamp: 2025-10-28T21:52:21.192Z
Learning: In Lychee version bump migrations (database/migrations), cache clearing failures should not prevent the migration from succeeding. The migration catches exceptions from `Artisan::call('cache:clear')`, logs a warning, and returns early instead of throwing the exception, allowing the version update to complete successfully.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3644
File: database/migrations/2025_08_24_122020_fix_timeline_config.php:25-29
Timestamp: 2025-08-24T14:33:37.747Z
Learning: In the Lychee codebase, ildyria prefers not to add null checks in migrations when they can guarantee that database keys exist, as indicated by their "Key exists, don't worry about it" response to null-safety suggestions in database/migrations/2025_08_24_122020_fix_timeline_config.php.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3637
File: database/migrations/2025_08_19_160144_create_renamer_rules_table.php:106-108
Timestamp: 2025-08-20T20:32:03.676Z
Learning: In the Lychee codebase, ildyria intentionally uses destructive migration patterns where up() calls down() to ensure clean state, as indicated by "Working as intended" and "No mercy" comments in migrations like database/migrations/2025_08_19_160144_create_renamer_rules_table.php.
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3637
File: database/migrations/2025_08_19_160144_create_renamer_rules_table.php:133-134
Timestamp: 2025-08-20T20:32:36.930Z
Learning: In the Lychee codebase, ildyria intentionally designs migrations to be destructive where up() calls down() first to ensure a clean state before inserting data. This approach makes migrations idempotent through destruction and recreation rather than through upsert operations, as confirmed by the "Working as intended" response to suggestions about using insertOrIgnore for config seeding.
📚 Learning: 2025-12-22T14:12:18.082Z
Learnt from: CR
Repo: LycheeOrg/Lychee PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T14:12:18.082Z
Learning: When both PHP and frontend files are modified, run the full quality gate: vendor/bin/php-cs-fixer fix, npm run format, npm run check, php artisan test, and make phpstan.
Applied to files:
docker/scripts/entrypoint.sh
📚 Learning: 2026-01-04T19:57:36.325Z
Learnt from: d7415
Repo: LycheeOrg/Lychee PR: 3932
File: docker/scripts/create-admin-user.sh:3-3
Timestamp: 2026-01-04T19:57:36.325Z
Learning: In docker/scripts/*.sh, enable strict Bash error handling by using 'set -euo pipefail'. This ensures the script exits on error, treats unset variables as errors, and propagates errors through pipes. Apply this to relevant scripts (e.g., create-admin-user.sh) to improve reliability and safety of Docker-related setup tasks.
Applied to files:
docker/scripts/entrypoint.sh
📚 Learning: 2025-10-28T21:52:21.192Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3774
File: database/migrations/2025_10_28_214205_bump_version060904.php:30-41
Timestamp: 2025-10-28T21:52:21.192Z
Learning: In Lychee version bump migrations (database/migrations), cache clearing failures should not prevent the migration from succeeding. The migration catches exceptions from `Artisan::call('cache:clear')`, logs a warning, and returns early instead of throwing the exception, allowing the version update to complete successfully.
Applied to files:
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php
📚 Learning: 2025-08-20T20:32:36.930Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3637
File: database/migrations/2025_08_19_160144_create_renamer_rules_table.php:133-134
Timestamp: 2025-08-20T20:32:36.930Z
Learning: In the Lychee codebase, ildyria intentionally designs migrations to be destructive where up() calls down() first to ensure a clean state before inserting data. This approach makes migrations idempotent through destruction and recreation rather than through upsert operations, as confirmed by the "Working as intended" response to suggestions about using insertOrIgnore for config seeding.
Applied to files:
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php
📚 Learning: 2025-08-20T20:32:03.676Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3637
File: database/migrations/2025_08_19_160144_create_renamer_rules_table.php:106-108
Timestamp: 2025-08-20T20:32:03.676Z
Learning: In the Lychee codebase, ildyria intentionally uses destructive migration patterns where up() calls down() to ensure clean state, as indicated by "Working as intended" and "No mercy" comments in migrations like database/migrations/2025_08_19_160144_create_renamer_rules_table.php.
Applied to files:
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php
📚 Learning: 2025-12-28T18:12:55.752Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3901
File: app/Providers/AppServiceProvider.php:0-0
Timestamp: 2025-12-28T18:12:55.752Z
Learning: When using Laravel Octane's tick API, Octane::tick(...) returns an InvokeTickCallable that only has ->seconds(int) and ->immediate() methods. There is no ->every(N) method. Use the correct usage: Octane::tick('name', fn() => ...)->seconds(N) or Octane::tick('name', fn() => ..., N). Apply this guideline to PHP files across the project (not just AppServiceProvider.php).
Applied to files:
database/migrations/2025_12_18_101617_preserve_size_variants_for_orders.php
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: 2️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 2️⃣ PHP dist / 8.5 - postgresql
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.4 - postgresql -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Webshop
🔇 Additional comments (1)
docker/scripts/entrypoint.sh (1)
103-109: LGTM! Explicit clearing before caching prevents stale data.The addition of
route:clearandview:clearbefore their respective cache commands follows the correct pattern for Laravel optimization. This ensures that any stale cached data is removed before rebuilding the cache, which directly addresses the PR objective to fix cache persistence of views.The sequence is now consistent:
- Clear old cache
- Build fresh cache
This pattern is applied uniformly across config, routes, and views.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.