Problem / Motivation
When a PDF file is generated for an invoice, we should store a reference to it instead of trying to figure out whether it exists via a query.
The patch from #3106014: Allow altering the invoice PDF filename via event subscribers is adding a field, but we're missing getters / setters as well as tests.
Also, the "challenging" part is that the field wasn't present before, which means if we're always trying get the file from the reference field, it won't work for existing invoices.
I'm not sure about the naming for that new field though, the patch from #3106014: Allow altering the invoice PDF filename via event subscribers went with "invoice_file".
To sum up, we need:
- A file reference field, translatable
- Getters/setters for that field (probably getFile(), setFile())
- Update savePrintable() to call a getFile() method that returns the file reference field if not empty, and fallback to the logic that we have today (In case a file is found, we need to set the reference.
Open questions:
- Should we delete the reference to the file on
Invoice::postDelete()? Or do we always want to keep the files?
Comments
Comment #2
jsacksick commentedComment #3
jsacksick commentedComment #4
jsacksick commentedComment #5
jsacksick commentedIdeally, we'd write a post update function to backfill the file reference, but what I did instead is that I populate the reference when the download url is accessed.
Comment #6
jsacksick commentedSame patch, with tweaked field descriptions.
Comment #8
jsacksick commentedCrediting archnode for his work in #3106014: Allow altering the invoice PDF filename via event subscribers.
Comment #9
jsacksick commentedI'm expanding the tests to ensure the file reference field is populated when downloading the file, and I fixed an issue with the logic in the invoice controller.
Comment #11
jsacksick commentedI went ahead and committed a slightly different patch with coding standard fixes (Removing unused use statements mainly, due to the tests refactoring).