Enable local file access in InvoicePdfFileGenerator #272
Conversation
coldic3
force-pushed
the
fix/wkhtmltopdf-local-file-access
branch
from
cb71b24 to
b640c6b
Compare
coldic3
force-pushed
the
fix/wkhtmltopdf-local-file-access
branch
2 times, most recently
from
9d8dff9 to
2363220
Compare
coldic3
force-pushed
the
fix/wkhtmltopdf-local-file-access
branch
from
2363220 to
264b909
Compare
|
Thank you, Kevin! 🎉 |
…oblems in the wkhtmltopdf (coldic3) This PR was merged into the 1.10 branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | 1.10 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | Sylius/RefundPlugin#361, Sylius/InvoicingPlugin#272 | License | MIT Commits ------- 56ed2a6 [Docs] Add a guide on how to deal with file access problems in the wkhtmltopdf
|
|
||
| use Symfony\Component\Config\FileLocatorInterface; | ||
|
|
||
| final class PdfOptionsGenerator implements PdfOptionsGeneratorInterface |
There was a problem hiding this comment.
What do you think about this naming?
| final class PdfOptionsGenerator implements PdfOptionsGeneratorInterface | |
| final class PdfOptionsWithAllowedFilesProvider implements PdfOptionsProviderInterface |
There was a problem hiding this comment.
If you go with "pdf options provider interface", then I would keep the interface name as an integral suffix, i.e.: "allowed files pdf options provider". As an extra improvement, I would move the "pdf" from class name to namespace.
| if (!isset($options['allow'])) { | ||
| $options['allow'] = []; | ||
| } elseif (!is_array($options['allow'])) { | ||
| $options['allow'] = [$options['allow']]; | ||
| } | ||
|
|
||
| $options['allow'] = array_merge( | ||
| $options['allow'], | ||
| array_map(fn ($file) => $this->fileLocator->locate($file), $this->allowedFiles) | ||
| ); | ||
|
|
||
| return $options; |
There was a problem hiding this comment.
Just a style suggestion:
| if (!isset($options['allow'])) { | |
| $options['allow'] = []; | |
| } elseif (!is_array($options['allow'])) { | |
| $options['allow'] = [$options['allow']]; | |
| } | |
| $options['allow'] = array_merge( | |
| $options['allow'], | |
| array_map(fn ($file) => $this->fileLocator->locate($file), $this->allowedFiles) | |
| ); | |
| return $options; | |
| $locatedFiles = array_map(fn ($file) => $this->fileLocator->locate($file), $this->allowedFiles); | |
| if (!isset($options['allow'])) { | |
| return $locatedFiles; | |
| } | |
| if (!is_array($options['allow'])) { | |
| $options['allow'] = [$options['allow']]; | |
| } | |
| $options['allow'] = array_merge( | |
| $options['allow'], | |
| $locatedFiles | |
| ); | |
| return $options; |
However, some cases are not covered by this class spec. For sure we do not cover the absence of $options['allow']. The case with $options['allow'] as the array is not covered as well. When can it happen?
It is not the case for spec probably, but what will happen if the file does not exist? What is returned by the fileLocator in such a case?
There was a problem hiding this comment.
Now $options['allow'] cannot be an array but it could be possible in the future because it makes sense so I have put this if statement to cover that case anyway 🤔
File locator throws an error for unexisting files.
| ) { | ||
| } | ||
|
|
||
| public function generate(): array |
There was a problem hiding this comment.
If you would follow naming change:
| public function generate(): array | |
| public function provide(): array |
Related ticket: Sylius/RefundPlugin#361