Invoice path relative to invoices_root_dir. Added invoice naming method. #5
Conversation
…dded invoice naming method.
| @@ -19,7 +19,12 @@ | |||
| /** | |||
| * @param InvoiceInterface $invoice | |||
| * | |||
| * @return string | |||
| * @return string Invoice filename | |||
| $invoice->getOrder()->getCreatedAt()->format('Ymd'), | ||
| bin2hex(random_bytes(6)), | ||
| ]; | ||
| return (string) implode('_', $tokens) . '.pdf'; |
There was a problem hiding this comment.
Add a blank line above the return statement
src/Resolver/InvoiceFileResolver.php
Outdated
|
|
||
| $invoice->setPath($invoicePath); | ||
| $invoiceFilename = $this->invoiceFileGenerator->generateFile($invoice); | ||
| $invoice->setPath($invoiceFilename); |
There was a problem hiding this comment.
Add a blank line above another type of algorithm operation. We separate associations, calls and input/output actions with a blank line ;)
| * @param InvoiceInterface $invoice | ||
| * @return string Returns an explicit invoice file name | ||
| */ | ||
| protected function getInvoiceFilename(InvoiceInterface $invoice): string |
There was a problem hiding this comment.
Can we move it to another service? As this is a vendor plugin, I think it's worth to allow decorating/overriding this method. If this is too much for you, please change it's visibility to private, as the service is a final class and extending services in the DI app is not the best practice. Please, move it also to the bottom of the class. We're ordering methods by their visibility, so public comes above protected and private are the last ones. 🙂
//edit. I also think the method name should be generateInvoiceFilename.
src/Resolver/InvoiceFileResolver.php
Outdated
| $this->invoiceEntityManager->flush(); | ||
| } | ||
|
|
||
| return $invoice->getPath(); | ||
| return $this->invoiceFileGenerator->getFilesPath() . DIRECTORY_SEPARATOR . $invoice->getPath(); |
There was a problem hiding this comment.
How about we name this method getFilesDirectoryPath()? I think this will be a more clear description of the method behavior 🙂.
|
|
||
| /** | ||
| * @param InvoiceInterface $invoice | ||
| * @return string Returns an explicit invoice file name |
There was a problem hiding this comment.
Please, add a blank line above @return doc.
.gitignore
Outdated
| @@ -9,3 +9,4 @@ | |||
| !/etc/build/.gitkeep | |||
|
|
|||
| /tests/Application/yarn.lock | |||
| /.idea | |||
There was a problem hiding this comment.
Please, remove it from the .gitignore. Configure a global .gitignore file instead. Read more here: https://gist.github.com/subfuzion/db7f57fff2fb6998a16c
| /** | ||
| * @var FilenameGeneratorInterface | ||
| */ | ||
| protected $filenameGenerator; |
There was a problem hiding this comment.
Please, mark it as private, the class is final, so there's no need to add protected fields to it.
src/Resources/config/services.yml
Outdated
| bitbag_sylius_invoicing_plugin.file_generator.invoice_file: | ||
| class: BitBag\SyliusInvoicingPlugin\FileGenerator\InvoicePdfFileGenerator | ||
| arguments: | ||
| - "@knp_snappy.pdf" | ||
| - "@templating" | ||
| - "@bitbag_sylius_invoicing_plugin.resolver.company_data" | ||
| - "%invoices_root_dir%" | ||
| - "@bitbag_sylius_invoicing_plugin.file_generator.invoice_filename" |
There was a problem hiding this comment.
In our convention, we put the object types above the default types, so if possible, interfaces over strings, arrays, etc. Could you please change this order remembering the right order in __construct call as well as properties order in InvoicePdfFileGenerator? 🙂
| string $filesPath, | ||
| FilenameGeneratorInterface $filenameGenerator | ||
| FilenameGeneratorInterface $filenameGenerator, | ||
| string $filesPath |
There was a problem hiding this comment.
Could you also change the order of declaration in __construct?
|
Thank you @ambroisemaupate! |
invoices_root_dirin order to store only important path info in database and make it portable.getInvoiceFilenamewhich can be overridden if you want to change it.getFilesPathgetter inFileGeneratorInterfaceto be able to get absolute invoices folder path from controllers.See issue #4
✌️Cheers!