Skip to content

Code quality improvements - #171

Open
CasperBE wants to merge 10 commits into
num-num:masterfrom
CasperBE:code-improvs
Open

Code quality improvements#171
CasperBE wants to merge 10 commits into
num-num:masterfrom
CasperBE:code-improvs

Conversation

@CasperBE

Copy link
Copy Markdown
Contributor

Added some improvements in the codebase, most are resolved warnings from PhpStorm.

  • Added missing libraries in composer.json file.
  • Update code where line length was exceeded.
  • Add missing parameters type declarations.
  • Add missing property's type declarations.
  • Changed non-English comments to English.

PHP DocBlock improvements:

  • Set correct argument type / return type.
  • Add throws tag where necessary.
  • Set correct parameter names.
  • Add missing PHP DocBlocks

- Update code where line length was exceeded.
- Add missing parameters type declarations.
- Add missing property's type declarations.
- Changed non-English comments to English.

PHP DocBlock improvements:
- Set correct argument type / return type.
- Add throws tag where necessary.
- Set correct parameter names.
- Add missing PHP DocBlocks
@PlayeMatthieu

Copy link
Copy Markdown
Contributor

Thanks for the cleanup work — the docblock corrections and line-length fixes are welcome. However, our verification run marks this PR as broken, so it needs further testing before it can be merged.

The property type declarations change runtime behaviour. Checking out this branch and master on the same machine with identical vendor/, running the repository's own read tests:

$ vendor/bin/phpunit tests/Read/ReadUbl.php
master        → OK (4 tests, 8 assertions)
code-improvs  → Tests: 4, Assertions: 2, Errors: 3

The three errors:

TypeError: NumNum\UBL\PartyTaxScheme::setRegistrationName(): Argument #1 ($registrationName)
           must be of type string, null given, called in src/PartyTaxScheme.php on line 122
TypeError: Cannot assign null to property NumNum\UBL\OrderReference::$salesOrderId of type string

These are real-world UBL documents (UBL-Invoice-2.1-Example.xml, UBL-CreditNote.xml, ubl-invoice-simple.xml). 5 of the 22 XML fixtures in the repo no longer parse. CI stays green only because tests/Read/ReadUbl.php lacks the Test filename suffix, so PHPUnit never loads it.

Root causes:

  1. src/PartyTaxScheme.php:32,50setRegistrationName(string) and setCompanyId(string) are non-nullable, but xmlDeserialize() (line 122-123) calls them with ?? null. Any invoice whose cac:PartyTaxScheme omits RegistrationName or CompanyID now throws. Please restore ?string.

  2. src/OrderReference.php:18private string $salesOrderId; is neither nullable nor defaulted. It breaks both directions: reading an invoice with cac:OrderReference but no cbc:SalesOrderID, and serializing an OrderReference where only setId() was called (a common pattern). Please use private ?string $salesOrderId = null;.

  3. src/TaxScheme.php:14private ?string $id; is nullable but has no default, so it stays uninitialized. getId() and serialization throw Error: must not be accessed before initialization where master returned null. Same for src/AllowanceCharge.php:15 (private bool $chargeIndicator;) and the static Generator::$currencyID / Reader::$currencyID. Every typed property here needs an explicit default, since these classes have no constructor.

Additional points, not fatal but worth addressing:

  • array $attributes = null in LegalEntity, PaymentMeans, Item and PartyIdentification is an implicitly nullable parameter — deprecated in PHP 8.4 and removed in PHP 9. The deprecation count in the suite goes from 18 to 28 with this branch. Please use ?array $attributes = null.
  • Invoice::$invoiceLines changed its default from null to [], so getInvoiceLines() / getCreditNoteLines() / getDebitNoteLines() now return [] instead of null. That is a behavioural break for any consumer doing a === null check.
  • PaymentMandate::$xmlTagName was removed. It is unused internally, but it was a public property.
  • ext-soap in require-dev does not appear to be used anywhere and would block CI runners without the extension.

Separately, and regardless of this PR: renaming tests/Read/ReadUbl.php to ReadUblTest.php would let CI catch this class of regression on its own.

Happy to re-run the verification once the type declarations are adjusted.

@CasperBE

Copy link
Copy Markdown
Contributor Author

Thank you for your verification.

I've done some further improvements, in order to get all tests green.

The ReadUbl test has been renamed to ReadUblTest so it runs when all tests are executed.

The ext-soap in require-dev has been added for the EN16931Test, which uses the SoapClient on line 218.

All other remarks are resolved in new commits in this PR.

I'm happy to do additional changes when needed.

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