-
Notifications
You must be signed in to change notification settings - Fork 5
Fix HTTP client exception handling, validation, consolidate documentation, and update AI guidelines #354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix HTTP client exception handling, validation, consolidate documentation, and update AI guidelines #354
Changes from 10 commits
021ed33
66b0080
8af979f
35695ac
141c12a
38e700c
5407f47
559d42d
865f82c
3867273
7821c4a
6c53fff
64e78f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,20 +20,24 @@ class FormatHandlerFactory | |||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||
| * Registry of available handlers. | ||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||
| * Currently limited to core formats to ensure stability and reliability. | ||||||||||||||||||||||||||||||||||||||||||||||
| * Supported formats: | ||||||||||||||||||||||||||||||||||||||||||||||
| * - CII: Cross Industry Invoice (UN/CEFACT standard, common in Germany/France) | ||||||||||||||||||||||||||||||||||||||||||||||
| * - UBL 2.1/2.4: Universal Business Language (most common for Peppol) | ||||||||||||||||||||||||||||||||||||||||||||||
| * - PEPPOL BIS 3.0: Default Peppol format for most countries | ||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||
| * Note: Other formats (EHF, Factur-X, Facturae, FatturaPA, OIOUBL, ZUGFeRD) | ||||||||||||||||||||||||||||||||||||||||||||||
| * have been temporarily removed pending implementation of their format handlers. | ||||||||||||||||||||||||||||||||||||||||||||||
| * Existing configurations using these formats will fall back to the recommended | ||||||||||||||||||||||||||||||||||||||||||||||
| * format for their country or PEPPOL BIS 3.0 as default. | ||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||
| * @var array<string, class-string<InvoiceFormatHandlerInterface>> | ||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||
| protected static array $handlers = [ | ||||||||||||||||||||||||||||||||||||||||||||||
| 'cii' => CiiHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'ehf_3.0' => EhfHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'factur-x' => FacturXHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'facturae_3.2' => FacturaeHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'fatturapa_1.2' => FatturapaHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'oioubl' => OioublHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'peppol_bis_3.0' => PeppolBisHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'ubl_2.1' => UblHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'ubl_2.4' => UblHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'zugferd_1.0' => ZugferdHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'zugferd_2.0' => ZugferdHandler::class, | ||||||||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -53,14 +57,18 @@ public static function create(PeppolDocumentFormat $format): InvoiceFormatHandle | |||||||||||||||||||||||||||||||||||||||||||||
| throw new RuntimeException("No handler available for format: {$format->value}"); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| /** @var BaseFormatHandler $handler */ | ||||||||||||||||||||||||||||||||||||||||||||||
| $handler = app($handlerClass); | ||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||
| /** @var BaseFormatHandler $handler */ | ||||||||||||||||||||||||||||||||||||||||||||||
| $handler = app($handlerClass); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // Set the format on the handler to ensure it matches what was requested | ||||||||||||||||||||||||||||||||||||||||||||||
| // This is especially important for handlers that can handle multiple formats (UBL, ZUGFeRD) | ||||||||||||||||||||||||||||||||||||||||||||||
| $handler->setFormat($format); | ||||||||||||||||||||||||||||||||||||||||||||||
| // Set the format on the handler to ensure it matches what was requested | ||||||||||||||||||||||||||||||||||||||||||||||
| // This is especially important for handlers that can handle multiple formats (UBL, ZUGFeRD) | ||||||||||||||||||||||||||||||||||||||||||||||
| $handler->setFormat($format); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| return $handler; | ||||||||||||||||||||||||||||||||||||||||||||||
| return $handler; | ||||||||||||||||||||||||||||||||||||||||||||||
| } catch (\Throwable $e) { | ||||||||||||||||||||||||||||||||||||||||||||||
| throw new RuntimeException("Failed to create handler for format: {$format->value}", 0, $e); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -88,22 +96,43 @@ public static function createForInvoice(Invoice $invoice): InvoiceFormatHandlerI | |||||||||||||||||||||||||||||||||||||||||||||
| $format = PeppolDocumentFormat::from($customer->peppol_format); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| return self::create($format); | ||||||||||||||||||||||||||||||||||||||||||||||
| } catch (ValueError $e) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Invalid format, continue to fallback | ||||||||||||||||||||||||||||||||||||||||||||||
| } catch (ValueError | RuntimeException $e) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Invalid format or handler not available, continue to fallback | ||||||||||||||||||||||||||||||||||||||||||||||
| \Illuminate\Support\Facades\Log::info("Customer's preferred Peppol format '{$customer->peppol_format}' is not available, falling back to recommended format", [ | ||||||||||||||||||||||||||||||||||||||||||||||
| 'customer_id' => $customer->id, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'invoice_id' => $invoice->id, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'country_code' => $countryCode, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'error' => $e->getMessage(), | ||||||||||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
nielsdrost7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // 2. Use mandatory format if required for country | ||||||||||||||||||||||||||||||||||||||||||||||
| $recommendedFormat = PeppolDocumentFormat::recommendedForCountry($countryCode); | ||||||||||||||||||||||||||||||||||||||||||||||
| if ($recommendedFormat->isMandatoryFor($countryCode)) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return self::create($recommendedFormat); | ||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||
| return self::create($recommendedFormat); | ||||||||||||||||||||||||||||||||||||||||||||||
| } catch (RuntimeException $e) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Mandatory format not available, fall through to default | ||||||||||||||||||||||||||||||||||||||||||||||
| \Illuminate\Support\Facades\Log::warning("Mandatory Peppol format '{$recommendedFormat->value}' for country '{$countryCode}' is not available, falling back to default", [ | ||||||||||||||||||||||||||||||||||||||||||||||
| 'invoice_id' => $invoice->id, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'country_code' => $countryCode, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'format' => $recommendedFormat->value, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'error' => $e->getMessage(), | ||||||||||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
nielsdrost7 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
117
to
131
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silent fallback for mandatory country formats may cause compliance issues. Per the relevant snippet, Consider either:
🔧 Option: Log warning when mandatory format unavailable // 2. Use mandatory format if required for country
$recommendedFormat = PeppolDocumentFormat::recommendedForCountry($countryCode);
if ($recommendedFormat->isMandatoryFor($countryCode)) {
try {
return self::create($recommendedFormat);
} catch (RuntimeException $e) {
- // Mandatory format not available, fall through to default
+ // Mandatory format not available - this may cause compliance issues
+ \Illuminate\Support\Facades\Log::warning(
+ "Mandatory format {$recommendedFormat->value} unavailable for country {$countryCode}, falling back to default"
+ );
}
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // 3. Try recommended format | ||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||
| return self::create($recommendedFormat); | ||||||||||||||||||||||||||||||||||||||||||||||
| } catch (RuntimeException $e) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Recommended format not available, use default | ||||||||||||||||||||||||||||||||||||||||||||||
| \Illuminate\Support\Facades\Log::info("Recommended Peppol format '{$recommendedFormat->value}' is not available, falling back to PEPPOL BIS 3.0", [ | ||||||||||||||||||||||||||||||||||||||||||||||
| 'invoice_id' => $invoice->id, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'country_code' => $countryCode, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'format' => $recommendedFormat->value, | ||||||||||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // 4. Fall back to default PEPPOL BIS | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing support for multiple format handlers (ehf_3.0, factur-x, facturae_3.2, fatturapa_1.2, oioubl, zugferd_1.0, zugferd_2.0) is a significant breaking change that could affect existing invoices and customers configured to use these formats. This should be documented in a migration guide, and there should be consideration for how existing data using these formats will be handled. The PR description mentions "Limited supported formats" but doesn't explain why these were removed or what happens to existing configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comprehensive documentation to the handler registry explaining:
See commit 3867273.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot I want the old formats back, backed by phpunit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored all format handlers to the registry! Added comprehensive PHPUnit tests for all 11 formats:
Test coverage includes handler creation, registry validation, and existence checks. See commit 7821c4a.