Allow altering the invoice PDF filename via event subscribers

Unfortunately, this isn't configurable atm (patches are welcome)... In the meantime, you could swap the commerce_invoice.print_builder service with your own, and just override the generateFilename() method.

The attached patch adds a hook to alter the filename on invoice generation.

Currently the generated filename is used to determine if an invoice already has an invoice file generated. This seems a bit brittle and could lead to orphaned files if the name generation changes. This patch also adds a new invoice file reference to the invoice entity that saves the file reference.

Updated patch to use entityreference field type.

+ * @param $file_name
+ * @param \Drupal\commerce_invoice\Entity\InvoiceInterface $

This misses the descriptoin of the params.

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "file" entity type does not exist.

Tests fail with this message. I'd guess that the new entity reference to files makes the file entity type to be required in tests now. Thus the test cases need to be updated to install/add the file entity type in the setup routine.

1. +++ b/commerce_invoice.api.php
   @@ -0,0 +1,27 @@
   + * Hooks and documentation related to paragraphs module.
   + */
   

   This mentions the paragraphs module.

2. +++ b/src/InvoicePrintBuilder.php
   @@ -76,23 +76,19 @@ class InvoicePrintBuilder implements InvoicePrintBuilderInterface {
   +      //return $invoice->invoice_file->entity;
   

   Why is this line commented out?

3. +++ b/src/InvoicePrintBuilder.php
   @@ -113,9 +112,10 @@ class InvoicePrintBuilder implements InvoicePrintBuilderInterface {
   +    $file_name = $base_file_name . '-' . $invoice->language()->getId() . '-' . str_replace('_', '', $invoice->getState()->getId()) . '.pdf';
   

   Here the $file_name has the .pdf suffix but yet we append it to the $file_name variable returned.

4. +++ b/src/InvoicePrintBuilder.php
   @@ -113,9 +112,10 @@ class InvoicePrintBuilder implements InvoicePrintBuilderInterface {
   +    \Drupal::moduleHandler()->invokeAll('commerce_invoice_filename_alter', [&$file_name, $invoice]);
   

   Commerce tends to use events rather than alter hooks, but maybe for this "simple" need we could make an exception, not sure.

   Also, for alter hooks, we're supposed to be using the alter() method and not invokeAll().

We should probably create a separate issue for adding the field that stores the reference to the invoice PDF, since that most probably requires writing a post update function for existing invoices...

Thank you for the reviews! - I updated the patch accordingly. Concerning the additional post update function: This is not needed for the functionality of this patch (as the reference will be generated if none is present).

Generally it could be handy to make sure that an invoice file exists for every invoice, no matter if it is ever explicitly downloaded.

I'm not against storing a reference to the invoice PDF, but altering the filename and adding the file reference field should be handled 2 separate issues.

Also, we don't really use alter hooks anywhere in Commerce. And we should inject the module handler if we decide to stick with the alter hook.

And we're also missing getters/setters for the field added + tests.

See #3129042: Store a reference to the generated PDF.

I switched the patch from a hook to an event. I tested locally but didn't add any test coverage specific to the event since I didn't see similar testing in Commerce core.

+++ b/src/Event/InvoiceFilenameEvent.php
@@ -0,0 +1,75 @@
+ * Defines the number format definition event.

oops :).

Patch looks good, we can test this pretty easily by adding an event subscriber in our test module, and then we can test the behavior in InvoicePrintBuilderTest.

I'm wondering if we should expand the event to allow overriding the print engine and/or the file scheme though I'm not sure that is necessary.

Committed, thanks everyone!