Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Adesin-fr
Copy link

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:

  • I have run composer run-script check --timeout=0 and no errors were reported
  • The new code is covered by unit tests (check build/coverage for coverage report)
  • I have updated the documentation to describe the changes

Copy link
Contributor

@waltertamboer waltertamboer left a 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",
Copy link
Contributor

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
Copy link
Contributor

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)) {
Copy link
Contributor

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);
Copy link
Contributor

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.

@PowerKiKi PowerKiKi changed the base branch from develop to master November 16, 2022 21:15
@Progi1984 Progi1984 force-pushed the master branch 3 times, most recently from 2d9f999 to e458249 Compare August 30, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants