From 8ab860a1ae3d3c9065328b44874880aa41fac8ee Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski <5760249+gdarko@users.noreply.github.com> Date: Sat, 1 Aug 2026 12:56:39 +0200 Subject: [PATCH] fix(pdf): make custom templates behave the way the docs describe (#730) Custom templates are a real feature with no test coverage at all, and several rough edges that only show up once someone actually uses one. make:template validated nothing. --type was checked only by the interactive prompt, so `--type=payment` skipped the prompt and died on an uncaught FileNotFoundException looking for payment1.blade.php: a stack trace instead of a message. The name was not checked either, so `../escaped` wrote outside the templates directory. Both are refused now. Every custom template of a type shared one partials/table.blade.php. It was written on first use and reused thereafter, so editing the items table for one custom template silently changed it for all of them -- a file that looks per-template and behaves globally. Each template now gets its own copy under partials/{name}/, and its include is rewritten to match. Existing templates keep including the old shared path, which still resolves. A custom template sharing a built-in's name appeared twice in the picker with the same label, and findFormattedTemplate() array_reverses and takes the first match, so the custom one silently won. The listing is keyed by name now, so it appears once, as the entry that will actually be used. A custom template with no same-named .png rendered in the picker: a blank tile, no error, no hint anything was missing. It falls back to the preview of the template it was cloned from. template_name was validated as `required` and nothing else, so any string was accepted and stored. findFormattedTemplate() returns null for an unknown name, the null reads as "not custom", and rendering falls through to app.pdf.{type}.{name} -- a raw "view not found" 500 at PDF time, long after the save that caused it. New PdfTemplateExists rule, scoped per document type. make:template also copies a _header/_footer companion when the source template has one, so a scaffolded template keeps the repeating page furniture. Both getEstimateTemplateName/getInvoiceTemplateName asked for the template list with the default image format, base64-encoding a preview of every template just to read the names back. Claude-Session: https://claude.ai/code/session_01QmECndmNZwzN65Zz9P87dF --- .../Commands/CreateTemplateCommand.php | 92 +++++++++++++------ app/Http/Requests/EstimatesRequest.php | 2 + app/Http/Requests/InvoicesRequest.php | 2 + app/Models/Estimate.php | 8 +- app/Models/Invoice.php | 7 +- app/Rules/PdfTemplateExists.php | 35 +++++++ app/Support/Pdf/PdfTemplateUtils.php | 27 ++++-- .../Feature/Pdf/CustomTemplateCommandTest.php | 92 +++++++++++++++++++ tests/Feature/Pdf/PdfTemplateListingTest.php | 43 +++++++++ .../Feature/Pdf/PdfTemplateValidationTest.php | 65 +++++++++++++ 10 files changed, 331 insertions(+), 42 deletions(-) create mode 100644 app/Rules/PdfTemplateExists.php create mode 100644 tests/Feature/Pdf/CustomTemplateCommandTest.php create mode 100644 tests/Feature/Pdf/PdfTemplateValidationTest.php diff --git a/app/Console/Commands/CreateTemplateCommand.php b/app/Console/Commands/CreateTemplateCommand.php index a47c60db..81c25dea 100644 --- a/app/Console/Commands/CreateTemplateCommand.php +++ b/app/Console/Commands/CreateTemplateCommand.php @@ -10,6 +10,14 @@ use Illuminate\Support\Str; class CreateTemplateCommand extends Command { + /** + * Document types that can be cloned. The --type option is checked against + * this rather than only the interactive prompt: passing an unsupported one + * used to skip the prompt and die on an uncaught FileNotFoundException + * further down, with a stack trace instead of a message. + */ + private const TYPES = ['invoice', 'estimate']; + /** * The name and signature of the console command. * @@ -22,17 +30,7 @@ class CreateTemplateCommand extends Command * * @var string */ - protected $description = 'Create estimate or invoice pdf template. '; - - /** - * Create a new command instance. - * - * @return void - */ - public function __construct() - { - parent::__construct(); - } + protected $description = 'Create estimate or invoice pdf template.'; /** * Execute the console command. @@ -43,7 +41,23 @@ class CreateTemplateCommand extends Command $templateType = $this->option('type'); if (! $templateType) { - $templateType = $this->choice('Create a template for?', ['invoice', 'estimate']); + $templateType = $this->choice('Create a template for?', self::TYPES); + } + + if (! in_array($templateType, self::TYPES, true)) { + $this->error(sprintf( + 'Unsupported template type "%s". Supported types: %s.', + $templateType, + implode(', ', self::TYPES) + )); + + return self::INVALID; + } + + if (! preg_match('/^[A-Za-z0-9._-]+$/', $templateName)) { + $this->error('Template name may only contain letters, numbers, dots, dashes and underscores.'); + + return self::INVALID; } if (PdfTemplateUtils::customTemplateFileExists($templateType, sprintf('%s.blade.php', $templateName))) { @@ -52,15 +66,26 @@ class CreateTemplateCommand extends Command return self::INVALID; } - if (! PdfTemplateUtils::toCustomTemplateMarkupFile( - Str::replace( - sprintf('app.pdf.%s', $templateType), - sprintf('pdf_templates::%s', $templateType), - Storage::disk('views')->get("/app/pdf/{$templateType}/{$templateType}1.blade.php"), - ), - $templateType, - $templateName - )) { + $source = Storage::disk('views')->get("/app/pdf/{$templateType}/{$templateType}1.blade.php"); + + // Point this template at its own copy of the shared partial before the + // blanket namespace rewrite below catches it. Previously every custom + // template of a type included the same partials/table.blade.php, which + // was written once and then reused, so editing the table for one custom + // template silently changed it for all of them. + $source = Str::replace( + sprintf('app.pdf.%s.partials.table', $templateType), + sprintf('pdf_templates::%s.partials.%s.table', $templateType, $templateName), + $source, + ); + + $source = Str::replace( + sprintf('app.pdf.%s', $templateType), + sprintf('pdf_templates::%s', $templateType), + $source, + ); + + if (! PdfTemplateUtils::toCustomTemplateMarkupFile($source, $templateType, $templateName)) { $this->error(sprintf('Unable to create %s template.', ucfirst($templateType))); return self::FAILURE; @@ -72,12 +97,25 @@ class CreateTemplateCommand extends Command $templateName, ); - if (! PdfTemplateUtils::customTemplateFileExists($templateType, 'partials/table.blade.php')) { - PdfTemplateUtils::toCustomTemplateFile( - Storage::disk('views')->get("/app/pdf/{$templateType}/partials/table.blade.php"), - $templateType, - 'partials/table.blade.php' - ); + PdfTemplateUtils::toCustomTemplateFile( + Storage::disk('views')->get("/app/pdf/{$templateType}/partials/table.blade.php"), + $templateType, + sprintf('partials/%s/table.blade.php', $templateName), + ); + + // Repeating page header/footer, if the source template has one. Named + // with the {template}_header / {template}_footer suffix the Gotenberg + // driver looks for. + foreach (['_header', '_footer'] as $suffix) { + $companion = "/app/pdf/{$templateType}/{$templateType}1{$suffix}.blade.php"; + + if (Storage::disk('views')->exists($companion)) { + PdfTemplateUtils::toCustomTemplateFile( + Storage::disk('views')->get($companion), + $templateType, + sprintf('%s%s.blade.php', $templateName, $suffix), + ); + } } $this->info( diff --git a/app/Http/Requests/EstimatesRequest.php b/app/Http/Requests/EstimatesRequest.php index 870e134c..333802bb 100644 --- a/app/Http/Requests/EstimatesRequest.php +++ b/app/Http/Requests/EstimatesRequest.php @@ -5,6 +5,7 @@ namespace App\Http\Requests; use App\Models\CompanySetting; use App\Models\Customer; use App\Models\Estimate; +use App\Rules\PdfTemplateExists; use App\Support\DocumentTotals; use Illuminate\Foundation\Http\FormRequest; use Illuminate\Validation\Rule; @@ -64,6 +65,7 @@ class EstimatesRequest extends FormRequest ], 'template_name' => [ 'required', + new PdfTemplateExists('estimate'), ], 'items' => [ 'required', diff --git a/app/Http/Requests/InvoicesRequest.php b/app/Http/Requests/InvoicesRequest.php index 561db0f3..d1a73524 100644 --- a/app/Http/Requests/InvoicesRequest.php +++ b/app/Http/Requests/InvoicesRequest.php @@ -5,6 +5,7 @@ namespace App\Http\Requests; use App\Models\CompanySetting; use App\Models\Customer; use App\Models\Invoice; +use App\Rules\PdfTemplateExists; use App\Support\DocumentTotals; use Illuminate\Foundation\Http\FormRequest; use Illuminate\Validation\Rule; @@ -63,6 +64,7 @@ class InvoicesRequest extends FormRequest ], 'template_name' => [ 'required', + new PdfTemplateExists('invoice'), ], 'items' => [ 'required', diff --git a/app/Models/Estimate.php b/app/Models/Estimate.php index e2c4864c..a815c945 100644 --- a/app/Models/Estimate.php +++ b/app/Models/Estimate.php @@ -296,11 +296,9 @@ class Estimate extends Model implements HasMedia { $templateName = Str::replace('estimate', 'invoice', $this->template_name); - $name = []; - - foreach (PdfTemplateUtils::getFormattedTemplates('invoice') as $template) { - $name[] = $template['name']; - } + // Empty image format: only the names are wanted here, and the default + // builds a base64 preview for every template to answer that. + $name = array_column(PdfTemplateUtils::getFormattedTemplates('invoice', ''), 'name'); if (in_array($templateName, $name) == false) { $templateName = 'invoice1'; diff --git a/app/Models/Invoice.php b/app/Models/Invoice.php index 3b9b0262..d4481951 100644 --- a/app/Models/Invoice.php +++ b/app/Models/Invoice.php @@ -303,10 +303,9 @@ class Invoice extends Model implements HasMedia { $templateName = Str::replace('invoice', 'estimate', $this->template_name); - $names = []; - foreach (PdfTemplateUtils::getFormattedTemplates('estimate') as $template) { - $names[] = $template['name']; - } + // Empty image format: only the names are wanted here, and the default + // builds a base64 preview for every template to answer that. + $names = array_column(PdfTemplateUtils::getFormattedTemplates('estimate', ''), 'name'); if (! in_array($templateName, $names)) { $templateName = 'estimate1'; diff --git a/app/Rules/PdfTemplateExists.php b/app/Rules/PdfTemplateExists.php new file mode 100644 index 00000000..97398251 --- /dev/null +++ b/app/Rules/PdfTemplateExists.php @@ -0,0 +1,35 @@ +templateType, ''), 'name'); + + if (! in_array($value, $names, true)) { + $fail("The selected :attribute is not an available {$this->templateType} template."); + } + } +} diff --git a/app/Support/Pdf/PdfTemplateUtils.php b/app/Support/Pdf/PdfTemplateUtils.php index c452ca6a..6a6c4776 100644 --- a/app/Support/Pdf/PdfTemplateUtils.php +++ b/app/Support/Pdf/PdfTemplateUtils.php @@ -64,15 +64,23 @@ class PdfTemplateUtils ); }); - return array_map(function ($file) use ($templateType, $imageFormat) { + $formatted = []; + + foreach ($files as $file) { $templateName = Str::before(basename($file['path']), '.blade.php'); if ($file['custom']) { $imagePath = self::getCustomTemplateFilePath($templateType, sprintf('%s.png', $templateName)); - $isCustomTemplate = true; + + // A custom template needs a same-named .png. Without one the + // picker used to render — a blank tile with no hint + // that anything was missing. Fall back to the preview of the + // template make:template clones from. + if (! File::exists($imagePath)) { + $imagePath = resource_path("static/img/PDF/{$templateType}1.png"); + } } else { $imagePath = resource_path('static/img/PDF/'.$templateName.'.png'); - $isCustomTemplate = false; } if (empty($imageFormat)) { @@ -83,12 +91,19 @@ class PdfTemplateUtils $imageValue = File::exists($imagePath) ? ImageUtils::toBase64Src($imagePath) : ''; } - return [ + // Keyed by name so a custom template that shares a built-in's name + // appears once rather than twice. Custom entries come last and so + // win, which matches what findFormattedTemplate() already resolved + // to — the picker just used to show both tiles with no way to tell + // which one you were clicking. + $formatted[$templateName] = [ 'name' => $templateName, 'path' => $imageValue, - 'custom' => $isCustomTemplate, + 'custom' => $file['custom'], ]; - }, $files); + } + + return array_values($formatted); } /** diff --git a/tests/Feature/Pdf/CustomTemplateCommandTest.php b/tests/Feature/Pdf/CustomTemplateCommandTest.php new file mode 100644 index 00000000..8541684c --- /dev/null +++ b/tests/Feature/Pdf/CustomTemplateCommandTest.php @@ -0,0 +1,92 @@ +path("{$type}/{$file}"); +} + +test('it clones a template that renders through the custom namespace', function () { + Artisan::call('make:template', ['name' => 'branded', '--type' => 'invoice']); + + expect(File::exists(customTemplatePath('invoice', 'branded.blade.php')))->toBeTrue(); + + $markup = File::get(customTemplatePath('invoice', 'branded.blade.php')); + + expect($markup)->not->toContain('app.pdf.invoice') + ->and($markup)->toContain('pdf_templates::invoice'); +}); + +/** + * Every custom template of a type used to include the same + * partials/table.blade.php, written once and then reused, so editing the table + * for one silently changed it for all of them. + */ +test('each template gets its own copy of the shared partial', function () { + Artisan::call('make:template', ['name' => 'first', '--type' => 'invoice']); + Artisan::call('make:template', ['name' => 'second', '--type' => 'invoice']); + + expect(File::exists(customTemplatePath('invoice', 'partials/first/table.blade.php')))->toBeTrue() + ->and(File::exists(customTemplatePath('invoice', 'partials/second/table.blade.php')))->toBeTrue(); + + expect(File::get(customTemplatePath('invoice', 'first.blade.php'))) + ->toContain('pdf_templates::invoice.partials.first.table'); +}); + +test('a preview image is written so the picker has something to show', function () { + Artisan::call('make:template', ['name' => 'branded', '--type' => 'invoice']); + + expect(File::exists(customTemplatePath('invoice', 'branded.png')))->toBeTrue(); +}); + +test('the new template shows up in the picker', function () { + Artisan::call('make:template', ['name' => 'branded', '--type' => 'invoice']); + + $names = array_column(PdfTemplateUtils::getFormattedTemplates('invoice', ''), 'name'); + + expect($names)->toContain('branded'); +}); + +/** + * --type was never checked against the supported list. An unsupported value + * skipped the interactive prompt and then died on an uncaught + * FileNotFoundException looking for e.g. payment1.blade.php. + */ +test('an unsupported type is refused with a message rather than a stack trace', function () { + $exit = Artisan::call('make:template', ['name' => 'receipt', '--type' => 'payment']); + + expect($exit)->toBe(Command::INVALID) + ->and(Artisan::output())->toContain('Unsupported template type'); +}); + +test('a name that would escape the templates directory is refused', function (string $name) { + $exit = Artisan::call('make:template', ['name' => $name, '--type' => 'invoice']); + + expect($exit)->toBe(Command::INVALID); +})->with([ + '../escaped', + 'nested/path', +]); + +test('an existing name is not overwritten', function () { + Artisan::call('make:template', ['name' => 'branded', '--type' => 'invoice']); + File::put(customTemplatePath('invoice', 'branded.blade.php'), 'EDITED BY USER'); + + $exit = Artisan::call('make:template', ['name' => 'branded', '--type' => 'invoice']); + + expect($exit)->toBe(Command::INVALID) + ->and(File::get(customTemplatePath('invoice', 'branded.blade.php')))->toBe('EDITED BY USER'); +}); diff --git a/tests/Feature/Pdf/PdfTemplateListingTest.php b/tests/Feature/Pdf/PdfTemplateListingTest.php index 303077ed..a8cc9cf7 100644 --- a/tests/Feature/Pdf/PdfTemplateListingTest.php +++ b/tests/Feature/Pdf/PdfTemplateListingTest.php @@ -51,3 +51,46 @@ test('a template whose name only contains the word is still listed', function () expect($names)->toContain('header_led_design'); }); + +/** + * A custom template sharing a built-in's name used to appear twice with the same + * label. findFormattedTemplate() array_reverses and takes the first match, so + * the custom one silently won -- the picker gave no indication which tile you + * were clicking. + */ +test('a custom template shadowing a built-in appears once, as the custom one', function () { + writeCustomTemplate($this->customDir, 'invoice1'); + + $templates = PdfTemplateUtils::getFormattedTemplates('invoice', ''); + $matching = array_values(array_filter($templates, fn ($t) => $t['name'] === 'invoice1')); + + expect($matching)->toHaveCount(1) + ->and($matching[0]['custom'])->toBeTrue(); +}); + +/** + * A custom template needs a same-named .png. Without one the picker rendered + * : a blank tile, no error, no hint anything was missing. + */ +test('a custom template with no preview falls back rather than rendering blank', function () { + writeCustomTemplate($this->customDir, 'no_preview'); + + $templates = PdfTemplateUtils::getFormattedTemplates('invoice'); + $entry = collect($templates)->firstWhere('name', 'no_preview'); + + expect($entry['path'])->toStartWith('data:image/png;base64,'); +}); + +test('a custom template with its own preview uses it', function () { + writeCustomTemplate($this->customDir, 'with_preview'); + File::put( + "{$this->customDir}/with_preview.png", + base64_decode('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==') + ); + + $stock = collect(PdfTemplateUtils::getFormattedTemplates('invoice'))->firstWhere('name', 'invoice1'); + $custom = collect(PdfTemplateUtils::getFormattedTemplates('invoice'))->firstWhere('name', 'with_preview'); + + expect($custom['path'])->toStartWith('data:image/png;base64,') + ->and($custom['path'])->not->toBe($stock['path']); +}); diff --git a/tests/Feature/Pdf/PdfTemplateValidationTest.php b/tests/Feature/Pdf/PdfTemplateValidationTest.php new file mode 100644 index 00000000..95dcdd74 --- /dev/null +++ b/tests/Feature/Pdf/PdfTemplateValidationTest.php @@ -0,0 +1,65 @@ + 'DatabaseSeeder', '--force' => true]); + Artisan::call('db:seed', ['--class' => 'DemoSeeder', '--force' => true]); + + $user = User::find(1); + $this->company = $user->companies()->first(); + $this->withHeaders(['company' => $this->company->id]); + Sanctum::actingAs($user, ['*']); +}); + +test('the rule accepts a template the picker offers', function () { + $validator = Validator::make( + ['template_name' => 'invoice1'], + ['template_name' => [new PdfTemplateExists('invoice')]] + ); + + expect($validator->fails())->toBeFalse(); +}); + +test('the rule rejects a template that does not exist', function () { + $validator = Validator::make( + ['template_name' => 'no-such-template'], + ['template_name' => [new PdfTemplateExists('invoice')]] + ); + + expect($validator->fails())->toBeTrue(); +}); + +/** + * The types are separate directories, so an estimate template is not a valid + * invoice template even though both exist. + */ +test('the rule is scoped to the document type', function () { + $validator = Validator::make( + ['template_name' => 'estimate1'], + ['template_name' => [new PdfTemplateExists('invoice')]] + ); + + expect($validator->fails())->toBeTrue(); +}); + +test('creating an invoice with an unknown template fails validation, not at render time', function () { + postJson('/api/v1/invoices', [ + 'invoice_date' => '2026-01-01', + 'due_date' => '2026-01-31', + 'customer_id' => 1, + 'template_name' => 'definitely-not-a-template', + 'items' => [], + ])->assertStatus(422)->assertJsonValidationErrors('template_name'); +});