Dev #1832
Dev #1832
Conversation
WalkthroughThis PR migrates the license management system from config-based flags to a runtime LicenseService provider, integrates license validation into the admin module controller and UI, optimizes Docker layering to reduce image size and improve caching, updates the mercator-core dependency, and enhances console command scheduling with named jobs and overlap guards. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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. 🔧 PHPStan (2.1.33)Installing dependencies from lock file (including require-dev)
... [truncated 7753 characters] ... p line 48: Source path "../mercator-core" is not found for package sourcentis/mercator install [--prefer-source] [--prefer-dist] [--prefer-install PREFER-INSTALL] [--dry-run] [--download-only] [--dev] [--no-suggest] [--no-dev] [--no-autoloader] [--no-progress] [--no-install] [--audit] [--audit-format AUDIT-FORMAT] [-v|vv|vvv|--verbose] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--apcu-autoloader-prefix APCU-AUTOLOADER-PREFIX] [--ignore-platform-req IGNORE-PLATFORM-REQ] [--ignore-platform-reqs] [--] [...] 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: 5
🧹 Nitpick comments (6)
app/Console/Kernel.php (2)
34-36: Inconsistent naming:mercator:certificate-expiracylacks a.name()call.Other scheduled commands use named jobs (e.g.,
cleanup-daily,cve-search-daily), but this one does not. Consider adding a name for consistency and easier identification in scheduler logs.🔎 Proposed fix
// Certificate expiration check $schedule->command('mercator:certificate-expiracy') + ->name('certificate-expiration-daily') ->daily();
44-53: Bothcpe:syncandlicense:checkare scheduled at 00:30.Running two potentially resource-intensive jobs at the exact same time may cause contention. Consider staggering them (e.g.,
dailyAt('00:45')for one) to avoid overlapping execution windows.🔎 Proposed fix
// Check license $schedule->command('license:check') - ->dailyAt('00:30') + ->dailyAt('00:45') ->name('validate-license-daily') ->withoutOverlapping(60);Dockerfile (2)
68-69: Silent failure suppression with|| truemay hide post-install issues.Using
|| truewill suppress any errors frompost-install-cmd, which could mask legitimate issues during image builds. Consider logging a warning or explicitly checking for expected failures.🔎 Suggested alternative
# Run composer scripts après avoir copié le code -RUN composer run-script post-install-cmd --no-interaction || true +RUN composer run-script post-install-cmd --no-interaction 2>&1 || echo "Warning: post-install-cmd failed (may be expected)"
64-65: Potentially redundant COPY for version.txt.Line 62 already copies the entire application directory (
. .), which should includeversion.txt. This additional COPY may be intentional to ensure the file is present, but it's redundant ifversion.txtis part of the repository.resources/views/modules.blade.php (1)
4-10: Minor: Inconsistent spelling of "Licence" vs "License".The button label uses British spelling "Check Licence" while other parts of the codebase (including the card header at Line 14) use "License". Consider standardizing on one spelling throughout the application.
resources/views/partials/sidebar.blade.php (1)
1-1: Cache license check result to avoid redundant service calls.The sidebar calls
app(LicenseService::class)->hasValidLicense()twice (Lines 553 and 568), which resolves the service container and executes the license check each time. This is inefficient and could impact performance if the license check involves any I/O operations.🔎 Proposed optimization
-@php use Mercator\Core\Services\LicenseService; @endphp +@php + $hasValidLicense = app(\Mercator\Core\Services\LicenseService::class)->hasValidLicense(); +@endphp <nav id="sidebar" class="sidebar"> <div class="search-box">Then update the conditional checks:
@can('module_manage') - @if (app(LicenseService::class)->hasValidLicense()) + @if ($hasValidLicense) <a href="{{ route("admin.modules.index") }}" class="ps-4 {{ request()->is('admin/modules*') ? 'active' : '' }}"> <i class="bi bi-plugin"></i><span class="menu-text">{{ trans('cruds.module.title_short') }}</span> </a> @endif @endcan<div class="sidebar-footer"> -@if (app(LicenseService::class)->hasValidLicense()) +@if ($hasValidLicense) Enterprise @else Open Source @endifAlso applies to: 553-553, 568-572
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
DockerfileROADMAP.mdapp/Console/Kernel.phpapp/Http/Controllers/Admin/ModuleController.phpcomposer.jsonconfig/app.phpguides/CODE_OF_CONDUCT.fr.mdguides/CODE_OF_CONDUCT.mdguides/CONTRIBUTING.fr.mdguides/CONTRIBUTING.mdguides/INSTALL.RedHat.fr.mdguides/INSTALL.RedHat.mdguides/INSTALL_VM.fr.mdguides/INSTALL_VM.mdpublic/build/assets/app-BNtKciM1.jspublic/build/manifest.jsonresources/css/custom.cssresources/js/app.jsresources/views/layouts/admin.blade.phpresources/views/modules.blade.phpresources/views/partials/sidebar.blade.phproutes/web.php
💤 Files with no reviewable changes (1)
- resources/css/custom.css
🧰 Additional context used
🧬 Code graph analysis (1)
routes/web.php (1)
app/Http/Controllers/Admin/ModuleController.php (1)
ModuleController(11-143)
🪛 LanguageTool
ROADMAP.md
[grammar] ~5-~5: Il y a peut-être une erreur ici
Context: ...ercator. Changements prévus en 2026 et plus : ## Evolutions majeures - [ ] Dessin de pro...
(QB_NEW_FR)
[grammar] ~38-~38: Il y a peut-être une erreur ici
Context: ...[ ] Dark Theme Changements réalisés en 2025 : ## Evolutions majeures - [x] Outil de dess...
(QB_NEW_FR)
[grammar] ~40-~40: Utilisez les accents correctement
Context: ...eme Changements réalisés en 2025 : ## Evolutions majeures - [x] Outil de dessin de la c...
(QB_NEW_FR_OTHER_ERROR_IDS_MISSING_ORTHOGRAPHY_DIACRITIC_ACUTE)
[style] ~45-~45: Ce verbe peut être considéré comme familier dans un contexte formel.
Context: ... [x] Générer un annuaire de crise - [x] Gérer les BIA dans Mercator : https://github....
(VERBES_FAMILIERS_PREMIUM)
[grammar] ~48-~48: Essayez « évolutions mineures »
Context: ...té du code avec LaraStan et PHPStan ## Evolutions mineurs - [x] Packaging des librairies...
(QB_NEW_FR_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~48-~48: Il y a peut-être une erreur ici
Context: ...avec LaraStan et PHPStan ## Evolutions mineurs - [x] Packaging des librairies JavaScript ...
(QB_NEW_FR)
🪛 markdownlint-cli2 (0.18.1)
ROADMAP.md
40-40: Multiple headings with the same content
(MD024, no-duplicate-heading)
45-45: Bare URL used
(MD034, no-bare-urls)
48-48: Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (8)
resources/js/app.js (1)
78-90: LGTM!Good use of optional chaining to guard against missing DOM elements. This prevents potential
TypeError: Cannot read properties of nullerrors when the shortcut buttons are not present on certain pages.public/build/manifest.json (1)
42-50: LGTM!The manifest correctly reflects the updated app.js bundle hash corresponding to the source code changes.
ROADMAP.md (1)
38-52: LGTM on the 2025 accomplishments section.Good documentation of completed features including the mapping drawing tool, Excel/CSV import, crisis directory, BIA management, and code quality improvements with LaraStan/PHPStan.
Dockerfile (1)
7-37: Good Docker layer optimization.The consolidated RUN block with virtual
.build-deps, combined with copying composer files before application code, effectively improves build caching and reduces final image size.routes/web.php (1)
434-435: LGTM!The new
/modules/checkroute is properly placed within the authenticated and authorized route group, following the existing naming convention. The GET method is appropriate for a license validation check that redirects back with status messages.config/app.php (1)
234-236: Service provider swap fromModulesServiceProvidertoLicenseServiceProvideris clean.The migration to
LicenseServiceProviderfrom mercator-core has been properly completed.ModulesServiceProvideris no longer registered in any configuration files, with only the orphaned class file remaining atapp/Providers/ModulesServiceProvider.php. Consider removing this unused file if it's no longer needed.resources/views/layouts/admin.blade.php (1)
25-25: LGTM: Bootstrap styling update.The change from
list-unstyledtolist-group-flushimproves visual consistency with the Bootstrap list-group components used in the new license UI elements.app/Http/Controllers/Admin/ModuleController.php (1)
66-68: Verify LicenseService return structure consistency.The view requires specific array keys from
$license:status,license_key,type,issued_date,modules(required), andexpiration_date,days_until_expiration(optional). The controller accesses$license['status']without defensive checks. EnsureLicenseService::getLicenseInfo()consistently returns this exact structure, including handling of error or null-license cases, so the view doesn't fail when accessing undefined keys.
| // Get license service | ||
| $licenseService = app(LicenseService::class); | ||
| // get the installed license | ||
| $license = $licenseService->getLicenseInfo(); | ||
|
|
||
| if ($license['status'] === 'expiring_soon') { | ||
| $message = "License is about to expire"; | ||
| } | ||
| else | ||
| $message = ""; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor message logic and add internationalization.
The message assignment logic has several issues:
- Hardcoded English message should use
trans()for internationalization - Awkward if-else structure with inconsistent formatting
- No error handling if
getLicenseInfo()fails or returns unexpected data - Empty string for non-expiring licenses is unclear semantically
🔎 Proposed refactor
// Get license service
$licenseService = app(LicenseService::class);
-// get the installed license
+
+// Get the installed license
$license = $licenseService->getLicenseInfo();
-if ($license['status'] === 'expiring_soon') {
- $message = "License is about to expire";
-}
-else
- $message = "";
-
+// Set warning message if license is expiring soon
+$message = ($license['status'] ?? null) === 'expiring_soon'
+ ? trans('cruds.module.messages.license_expiring')
+ : null;
// Modules vus via ComposerAdd the translation key to your language files:
// resources/lang/en/cruds.php
'module' => [
'messages' => [
'license_expiring' => 'License is about to expire',
],
],📝 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.
| // Get license service | |
| $licenseService = app(LicenseService::class); | |
| // get the installed license | |
| $license = $licenseService->getLicenseInfo(); | |
| if ($license['status'] === 'expiring_soon') { | |
| $message = "License is about to expire"; | |
| } | |
| else | |
| $message = ""; | |
| // Get license service | |
| $licenseService = app(LicenseService::class); | |
| // Get the installed license | |
| $license = $licenseService->getLicenseInfo(); | |
| // Set warning message if license is expiring soon | |
| $message = ($license['status'] ?? null) === 'expiring_soon' | |
| ? trans('cruds.module.messages.license_expiring') | |
| : null; |
🤖 Prompt for AI Agents
In app/Http/Controllers/Admin/ModuleController.php around lines 22 to 32,
replace the hardcoded, oddly formatted if/else with a concise, safe assignment
that uses trans() for internationalization and handles missing or unexpected
license data: call getLicenseInfo(), verify it returned an array/object and
fetch the status safely (use null-coalescing or guard), set $message to
trans('cruds.module.messages.license_expiring') only when status ===
'expiring_soon', otherwise set $message to null (not an empty string), and add
minimal error handling/logging if getLicenseInfo() fails so the controller won't
throw on invalid data.
| public function check(): RedirectResponse | ||
| { | ||
| // Get license service | ||
| $licenseService = app(LicenseService::class); | ||
| $errors = collect(); | ||
|
|
||
| // Validation avec le serveur de licences | ||
| if ($licenseService->validateWithServer()) { | ||
| $message = ('License has been validated with the Licence server.'); | ||
|
|
||
| } else { | ||
| $message = ""; | ||
| $errors->add("License could not be verified with the Licence server !"); | ||
| } | ||
|
|
||
| return back() | ||
| ->with('message', $message) | ||
| ->with('errors', $errors); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add internationalization and simplify error handling.
The check() method has several issues:
- Messages are hardcoded in English instead of using
trans()for internationalization - Line 110: Inconsistent punctuation (period inside parentheses)
- Using
collect()for a single error message is unnecessary overhead - No exception handling if
validateWithServer()throws an error
🔎 Proposed refactor
public function check(): RedirectResponse
{
- // Get license service
- $licenseService = app(LicenseService::class);
- $errors = collect();
-
- // Validation avec le serveur de licences
- if ($licenseService->validateWithServer()) {
- $message = ('License has been validated with the Licence server.');
-
- } else {
- $message = "";
- $errors->add("License could not be verified with the Licence server !");
- }
-
- return back()
- ->with('message', $message)
- ->with('errors', $errors);
+ try {
+ $licenseService = app(LicenseService::class);
+
+ if ($licenseService->validateWithServer()) {
+ return back()->with('message', trans('cruds.module.messages.license_validated'));
+ }
+
+ return back()->withErrors([trans('cruds.module.errors.license_validation_failed')]);
+ } catch (\Exception $e) {
+ return back()->withErrors([trans('cruds.module.errors.license_check_error', ['error' => $e->getMessage()])]);
+ }
}Add translation keys:
// resources/lang/en/cruds.php
'module' => [
'messages' => [
'license_validated' => 'License has been validated with the license server.',
],
'errors' => [
'license_validation_failed' => 'License could not be verified with the license server.',
'license_check_error' => 'License check failed: :error',
],
],📝 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 function check(): RedirectResponse | |
| { | |
| // Get license service | |
| $licenseService = app(LicenseService::class); | |
| $errors = collect(); | |
| // Validation avec le serveur de licences | |
| if ($licenseService->validateWithServer()) { | |
| $message = ('License has been validated with the Licence server.'); | |
| } else { | |
| $message = ""; | |
| $errors->add("License could not be verified with the Licence server !"); | |
| } | |
| return back() | |
| ->with('message', $message) | |
| ->with('errors', $errors); | |
| } | |
| public function check(): RedirectResponse | |
| { | |
| try { | |
| $licenseService = app(LicenseService::class); | |
| if ($licenseService->validateWithServer()) { | |
| return back()->with('message', trans('cruds.module.messages.license_validated')); | |
| } | |
| return back()->withErrors([trans('cruds.module.errors.license_validation_failed')]); | |
| } catch (\Exception $e) { | |
| return back()->withErrors([trans('cruds.module.errors.license_check_error', ['error' => $e->getMessage()])]); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/Http/Controllers/Admin/ModuleController.php around lines 102–120, replace
hardcoded strings and the unnecessary collect() with translated messages and add
exception handling: wrap the call to $licenseService->validateWithServer() in a
try/catch, set $message = trans('cruds.module.messages.license_validated') on
success, on validation failure set a simple error string using
trans('cruds.module.errors.license_validation_failed'), and on exception set an
error string using trans('cruds.module.errors.license_check_error', ['error' =>
$e->getMessage()]) and log the exception; remove collect() and return using
back()->with('message', $message)->withErrors([$error]) (or
withErrors($errorsArray)) ensuring no stray punctuation inside parentheses and
translated keys are used.
| "phpoffice/phpword": "^1.4", | ||
| "socialiteproviders/keycloak": "^5.3", | ||
| "sourcentis/mercator-core": "^1.11", | ||
| "sourcentis/mercator-core": "^1.21", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
sourcentis mercator-core changelog versions 1.11 1.21
💡 Result:
I found a 1.11.0 release for sourcentis/mercator-core (published on Packagist 2025-12-08). I could not find any mercator-core version 1.21 (no 1.21 tag or release in the repository or on Packagist). Sources: Packagist (package page showing v1.11.0). [1] GitHub repository (no 1.21 release/tag present). [2]
If you want, I can fetch the 1.11.0 changelog/commit notes from the repo and paste the changes.
Sources
[1] Packagist — sourcentis/mercator-core (shows v1.11.0). (packagist.org)
[2] GitHub — dbarzin/mercator-core (repository/tags/releases). (github.com)
Citations:
- 1: https://packagist.org/packages/sourcentis/mercator-core
- 2: https://github.com/dbarzin/mercator-core
🌐 Web query:
sourcentis mercator-core latest version available releases
💡 Result:
The latest sourcentis/mercator-core release is version 1.4.0 (published 2025-12-01). Sources: Packagist listing and the project GitHub releases. [1][2]
Sources:
[1] Packagist — sourcentis/mercator-core (shows v1.4.0, 2025-12-01).
[2] GitHub — mercator-core releases (project repository/releases).
Version ^1.21 of sourcentis/mercator-core does not exist.
This constraint cannot be resolved by Composer. The available versions of mercator-core do not include 1.21. Verify the intended version and correct the constraint in composer.json.
🤖 Prompt for AI Agents
In composer.json at line 22, the dependency "sourcentis/mercator-core": "^1.21"
references a non-existent version; check Packagist or the mercator-core Git tags
to determine the correct published version (for example the latest 1.x tag),
replace "^1.21" with a valid version constraint (e.g. "^1.2" or the exact tag
you found), then run composer validate and composer update (or composer require
with the corrected constraint) to ensure the dependency can be resolved.
| <tr><th>Status</th><td>{{ $license['status'] }}</td></tr> | ||
| <tr><th>Key</th><td>{{ $license['license_key'] }}</td></tr> | ||
| <tr><th>Type</th><td>{{ $license['type'] }}</td></tr> | ||
| <tr><th>Issued Date</th><td>{{ $license['issued_date'] }}</td></tr> | ||
| <tr><th>Expiration Date</th><td>{{ $license ['expiration_date'] ?? 'Never' }}</td></tr> | ||
| <tr><th>Days Until Expiration </th><td>{{ $license['days_until_expiration'] ?? 'N/A' }}</td></tr> | ||
| <tr><th>Modules</th><td>{{ implode(', ', $license['modules']) }}</td></tr> | ||
| </table> |
There was a problem hiding this comment.
Add defensive checks for license data.
The template directly accesses $license array keys without verifying the array exists or contains the expected keys. If the controller fails to provide license data or returns an incomplete array, this will cause runtime errors.
🔎 Proposed defensive checks
+@if(isset($license) && is_array($license))
<div class="card">
<div class="card-header">
License
</div>
<div class="card-body">
<div class="col-4">
<table class="table table-bordered">
- <tr><th>Status</th><td>{{ $license['status'] }}</td></tr>
- <tr><th>Key</th><td>{{ $license['license_key'] }}</td></tr>
- <tr><th>Type</th><td>{{ $license['type'] }}</td></tr>
- <tr><th>Issued Date</th><td>{{ $license['issued_date'] }}</td></tr>
- <tr><th>Expiration Date</th><td>{{ $license ['expiration_date'] ?? 'Never' }}</td></tr>
- <tr><th>Days Until Expiration </th><td>{{ $license['days_until_expiration'] ?? 'N/A' }}</td></tr>
- <tr><th>Modules</th><td>{{ implode(', ', $license['modules']) }}</td></tr>
+ <tr><th>Status</th><td>{{ $license['status'] ?? 'Unknown' }}</td></tr>
+ <tr><th>Key</th><td>{{ $license['license_key'] ?? 'N/A' }}</td></tr>
+ <tr><th>Type</th><td>{{ $license['type'] ?? 'Unknown' }}</td></tr>
+ <tr><th>Issued Date</th><td>{{ $license['issued_date'] ?? 'N/A' }}</td></tr>
+ <tr><th>Expiration Date</th><td>{{ $license['expiration_date'] ?? 'Never' }}</td></tr>
+ <tr><th>Days Until Expiration</th><td>{{ $license['days_until_expiration'] ?? 'N/A' }}</td></tr>
+ <tr><th>Modules</th><td>{{ isset($license['modules']) && is_array($license['modules']) ? implode(', ', $license['modules']) : 'None' }}</td></tr>
</table>
</div>
</div>
</div>
+@else
+ <div class="alert alert-warning">License information is not available.</div>
+@endif
<br>Note: Also fixed the spacing issue on Line 23 ($license ['expiration_date'] → $license['expiration_date']) and removed trailing space after "Expiration" on Line 24.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In resources/views/modules.blade.php around lines 19 to 26, the template
directly indexes $license which can cause runtime errors if $license is null or
missing keys; update the template to first check that $license is set and is an
array (or fallback to an empty array) and replace direct accesses with
null-coalescing expressions (e.g. $license['status'] ?? 'N/A'), ensure the
expiration_date spacing is fixed to $license['expiration_date'] and use a safe
check for modules (e.g. is_array($license['modules']) ? implode(', ',
$license['modules']) : 'None') so each row renders a sensible default instead of
throwing an error.
| - [x] Gérer les BIA dans Mercator : https://github.com/dbarzin/mercator/discussions/1375 | ||
| - [x] Améliorer la qualité du code avec LaraStan et PHPStan | ||
|
|
||
| ## Evolutions mineurs |
There was a problem hiding this comment.
Minor typo: "Evolutions mineurs" should be "Evolutions mineures".
French adjective agreement requires "mineures" (feminine plural) to match "Evolutions" (feminine noun).
🔎 Proposed fix
-## Evolutions mineurs
+## Evolutions mineures📝 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.
| ## Evolutions mineurs | |
| ## Evolutions mineures |
🧰 Tools
🪛 LanguageTool
[grammar] ~48-~48: Essayez « évolutions mineures »
Context: ...té du code avec LaraStan et PHPStan ## Evolutions mineurs - [x] Packaging des librairies...
(QB_NEW_FR_OTHER_ERROR_IDS_REPLACEMENT_MULTITOKEN)
[grammar] ~48-~48: Il y a peut-être une erreur ici
Context: ...avec LaraStan et PHPStan ## Evolutions mineurs - [x] Packaging des librairies JavaScript ...
(QB_NEW_FR)
🪛 markdownlint-cli2 (0.18.1)
48-48: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
In ROADMAP.md around line 48, the heading "Evolutions mineurs" has a French
agreement typo; change "mineurs" to "mineures" so the adjective agrees in gender
and number with the feminine plural noun "Evolutions" (update the heading to
"Evolutions mineures").
Summary by CodeRabbit
New Features
Documentation
Style
✏️ Tip: You can customize this high-level summary in your review settings.