Skip to content

Update PaymentMandate - #174

Open
ronaldsgailis wants to merge 1 commit into
num-num:masterfrom
ronaldsgailis:fix/payment_mandate
Open

Update PaymentMandate#174
ronaldsgailis wants to merge 1 commit into
num-num:masterfrom
ronaldsgailis:fix/payment_mandate

Conversation

@ronaldsgailis

@ronaldsgailis ronaldsgailis commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Remove nonexistent PayeeFinancialAccount property and add PayerFinancialAccount property.

@PlayeMatthieu

Copy link
Copy Markdown
Contributor

Thanks for this — the premise is correct and the change is valuable. UBL 2.1 cac:PaymentMandate indeed has no PayeeFinancialAccount child (it has ID, MandateTypeCode, PayerParty, PayerFinancialAccount, …), so the old code emitted schema-invalid XML. And PaymentMeans/PaymentMandate/PayerFinancialAccount/cbc:ID is exactly EN 16931 BT-91 (debited account for SEPA direct debit), so this fills a real gap.

I checked out the branch and tested it. The write path produces correct XML, but there are three issues I'd like to see fixed before merging.

1. Blocker — PayerFinancialAccount is not registered in Reader::$elementMap

src/Reader.php maps every CAC element to its deserializer. Without an entry, sabre/xml hands back a raw array instead of an object, so reading any document with a payer account throws:

TypeError: PaymentMandate::setPayerFinancialAccount():
Argument #1 ($payerFinancialAccount) must be of type ?NumNum\UBL\PayerFinancialAccount, array given
called in src/PaymentMandate.php on line 60

Worth noting this is a regression, not a pre-existing bug: on master the reader silently ignores PayerFinancialAccount (the PayeeFinancialAccount key simply isn't present, so it resolves to null). After this PR, parsing any incoming direct-debit UBL throws. That breaks consumers that read inbound invoices with this library.

Please add, next to the Payee line:

Schema::CAC.        'PayerFinancialAccount'       => fn ($reader) => PayerFinancialAccount::xmlDeserialize($reader),

2. Blocker — PayerFinancialAccount::setId() / getId() are not nullable

setId(string $id) is non-nullable, but xmlDeserialize() calls ->setId($keyValues[Schema::CBC . 'ID'] ?? null). Since cbc:ID is optional in UBL, this throws on any mandate without an account ID:

TypeError: setId(): Argument #1 ($id) must be of type string, null given
TypeError: getId(): Return value must be of type string, null returned

Both should be ?string, matching PayeeFinancialAccount.

3. Blocker — xmlSerialize() always writes <cbc:ID>

There is no null guard, so an empty PayerFinancialAccount serializes an empty <cbc:ID/> element, which is schema-invalid. PayeeFinancialAccount::xmlSerialize() does guard this. Please add:

if ($this->id !== null) {
    // write ID
}

Non-blocking notes

  • BC break. Removing the public getPayeeFinancialAccount() / setPayeeFinancialAccount() methods and the public $xmlTagName property breaks external consumers. Internally it's safe — PaymentMeans hardcodes the PaymentMandate tag name and nothing reads PaymentMandate::$xmlTagName — but it deserves a changelog entry / a major version.
  • Schema::CAC . $this->payerFinancialAccount->xmlTagName deviates from the rest of the library, which hardcodes the tag name everywhere. Schema::CAC . 'PayerFinancialAccount' would be more consistent, and drops the dependency on a public property.
  • PayerFinancialAccount omits Name and FinancialInstitutionBranch, which exist in UBL and are present on PayeeFinancialAccount. Peppol BIS doesn't need them, so a minimal class is fine, but the asymmetry is worth a conscious decision.
  • No tests. tests/Write and tests/Read both exist, and PaymentMandate had zero coverage until now. A single round-trip test would have caught issues 1 and 2 — could you add one?
  • The PHPDoc blocks were dropped while every sibling class still carries them. Purely stylistic, no objection to the content.

Could you address the three blockers (Reader registration, nullable ID, null guard on serialize) plus a round-trip test? Happy to merge after that — the direction is right and it fixes genuine schema invalidity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants