-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TemplateProcessor can accept a file contents instead of the file path #2119
base: master
Are you sure you want to change the base?
Conversation
…e path to the file.
…e path to the file.
…e path to the file.
…e path to the file. Added doc and test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to help by reviewing the PR.
I do understand the motivation for making this change, yet I would recommend a different approach. Instead of providing the path to a file or the content of a file, it would be better to approach this from a more OOP kind of way:
interface TemplateProcessorFileLoader
{
/**
* Loads the file and returns a stream to the file that should be processed.
*
* @return string Returns a stream to the file content.
*/
public function load(): resource;
}
class TemplateProcessor
{
// ... snip ...
public function __construct(TemplateProcessorFileLoader $loader)
{
$this->tempDocumentFilename = $this->loadFile($loader);
// snip
}
private function loadFile(TemplateProcessorFileLoader $loader): string
{
$tempDocumentFilename = tempnam(Settings::getTempDir(), 'PhpWord');
if (false === $tempDocumentFilename) {
throw new CreateTemporaryFileException(); // @codeCoverageIgnore
}
// Something like this or figure out whatever approach works best...
$handle = fopen($tempDocumentFilename, 'wb');
// ... error handling ...
stream_copy_to_stream($loader->load(), $handle);
fclose($handle);
return $tempDocumentFilename;
}
Nevertheless it might be a good idea to merge this PR since it does add more flexibility and create an issue to refactor this for a next major version.
composer.json
Outdated
@@ -1,5 +1,5 @@ | |||
{ | |||
"name": "phpoffice/phpword", | |||
"name": "lemarinelnet/phpword", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like you accidentally committed this. The composer project name cannot change obviously.
@@ -0,0 +1,23 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, more examples are always a good thing. Nice! :-)
if (false === copy($documentTemplate, $this->tempDocumentFilename)) { | ||
throw new CopyFileException($documentTemplate, $this->tempDocumentFilename); // @codeCoverageIgnore | ||
// If we provided a file name, use it : | ||
if (@file_exists($documentTemplate)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think errors should be suppressed. As far as I can read from the documentation and doing a quick test, when the file does not exist, false
is returned. No warning should occur.
{ | ||
$contents = file_get_contents(__DIR__ . '/_files/templates/blank.docx'); | ||
$object = new TemplateProcessor($contents); | ||
$this->assertInstanceOf('PhpOffice\\PhpWord\\TemplateProcessor', $object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assert doesn't make sense. Of course it's going to be an instance of the class you instantiate :-) Can be removed.
2d9f999
to
e458249
Compare
Description
In many projects, Devs are now using Cloud storage (such as AWS S3).
So, having a word template in a S3 storage would force the dev to save it to a custom/temporary path and then give the path to the TemplateProcessor constructor.
I submit this PR so the constructor can ALSO accept the file contents.
If the given parameter is a path to a valid file, then the constructor works as before. Else, we suppose that the parameter contains the file contents, and then we save it to the tempDocumentFilename, and use it as we did before.
Checklist:
composer run-script check --timeout=0
and no errors were reported