Remove PDF files after generation#16401
Remove PDF files after generation#16401magento-engcom-team merged 5 commits intomagento:2.2-developfrom
Conversation
magento-engcom-team
added
Partner: Atwix
|
Hi @rogyar. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
| */ | ||
| private function getFileContent($content) | ||
| { | ||
| if (isset($content['type']) && $content['type'] == 'string') { |
There was a problem hiding this comment.
We should check if $content is an array before checking the isset. Not sure that PHP will not complain about this.
In this case is not strictly required, but I suggest always to use the === strict comparison.
There was a problem hiding this comment.
isset wont generate a notice in that case if the index does not exist. Also, it's applicable not only to arrays, the function checks that the variable is set. So, not sure we should use is_array here, which may be more expensive and redundant.
As for the strict comparison, absolutely makes sense, thank you.
| ->setHeader('Content-Length', $contentLength === null ? strlen($content) : $contentLength, true) | ||
| ->setHeader( | ||
| 'Content-Length', | ||
| $contentLength === null ? strlen($this->getFileContent($content)) : $contentLength, |
There was a problem hiding this comment.
Comment on code readability:
Since you use getFileContent twice (line 81 and 95), why not to put the result in a variable?
There was a problem hiding this comment.
Agree, the mentioned approach improves the readability, thanks.
| * @param string|array $content | ||
| * @return string | ||
| */ | ||
| private function getFileContent($content) |
There was a problem hiding this comment.
Please use type hints in method signatures both for parameters and return types. Looks like only return type applies to this specific case
There was a problem hiding this comment.
Sure, I always go this way for a new classes. I had doubts about methods within a scope of existing classes with no type hints/strict types.
There was a problem hiding this comment.
@rogyar , you are right if it was public because of possible plugins on it not matching the signature, but with a private method is definitely safe.
ishakhsuvarov
left a comment
ishakhsuvarov
left a comment
There was a problem hiding this comment.
It would be great to accompany this PR with a test, preferably integration one
|
@ishakhsuvarov absolutely. I'm going to do that as well as adjust packingslips/credit memo generation. Just wanted to make sure that we go the right way. Thank you. |
|
LGTM, just check tests. |
|
Finally, handled the failing tests :) |
|
Hi @phoenix128, thank you for the review. |
|
Hi @rogyar. Thank you for your contribution. Please, consider to port this solution to 2.3 release line. |
4 participants
Description
Upon invoice/packingslip/credit memo printing the system generates a PDF file directly in the
vardirectory. I see no reason for keeping these files since they are not accessible publicly via web (sharing purpose). There's no "reuse" purpose as well since on every print action a new file with a new filename is being generated.This PR provides a logic for removing a PDF file once it's generated.
Currently, it's implemented only within a scope of the invoice printing. Once we agree on the solution, I will adjust other places with PDF generation.
Fixed Issues (if relevant)
Manual testing scenarios
vardirectory.