Skip to content

Get rid of duplicated code in TemplateProcessor.php #1161

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

Merged
merged 2 commits into from
Dec 26, 2018

Conversation

abcdmitry
Copy link
Contributor

No description provided.

@FBnil
Copy link

FBnil commented Oct 19, 2017

Why not self::ensureMacroCompleted like in the function setValue?

... And thank you for helping out.

@abcdmitry
Copy link
Contributor Author

This is a late static binding feature allowing to overwrite ensureMacroCompleted method from the child class.
Right now I work on extending this class within my project and I need to overwrite this method.

Actually replacing self:: with static:: within the setValue() would be nice as well, since it doesn't affect current functionality, but offers flexibility for those willing to extend the class.

@FBnil
Copy link

FBnil commented Oct 19, 2017

Aha.. I did a complete rewrite of TemplateProcessor .
Maybe this will be useful:

Enable/disable setValue('key') becoming setValue('${key}') automatically.
Call it like: TemplateProcessor::$ensureMacroCompletion = false;

@FBnil
Copy link

FBnil commented Oct 19, 2017

While you are at cleanups, do not forget:

in function setValue():

        $this->tempDocumentMainPart = $this->setValueForPart($search, $replace, $this->tempDocumentMainPart, $limit);

        $documentParts = array('tempDocumentHeaders', 'tempDocumentFooters');
        foreach ($documentParts as $tempDocumentParts) {
            foreach ($this->{$tempDocumentParts} as &$tempDocumentPart) {
                $tempDocumentPart = $this->setValueForPart($search, $replace, $tempDocumentPart, $limit);
            }
        }

Because the Headers and Footers are arrays. (current code works, but Travis complains about it, even with explicit typecast, but it's been a while and maybe it works)
Possible minimal change:

        $this->tempDocumentHeaders = $this->setValueForPart($search, $replace, (array)$this->tempDocumentHeaders, $limit);
        $this->tempDocumentMainPart = $this->setValueForPart($search, $replace, (string)$this->tempDocumentMainPart, $limit);
        $this->tempDocumentFooters = $this->setValueForPart($search, $replace, (array)$this->tempDocumentFooters, $limit);

Yeah, I am fairly confident that by changing * @param string $documentPartXML in setValueForPart() to * @param mixed $documentPartXML (including the @return value) it should work. Giving that a try.

I did not bother too much with unset, the current code does not bite itself, and when the function ends, they are out of scope and cleaned up automatically. It IS, however, good practice.

Unfortunately, the is_array can not be joined, as you are doing different things:

>>> str_replace(['a','v'],['x'],"Laravel")
=> "Lxrxel"
>>> str_replace(['a','v'],'x',"Laravel")
=> "Lxrxxel"
>>> preg_replace(['/a/','/v/'],'x',"Laravel")
=> "Lxrxxel"
>>> preg_replace(['/a/','/v/'],['x'],"Laravel")
=> "Lxrxel"

@abcdmitry
Copy link
Contributor Author

I am not sure where the code foreach ($this->{$tempDocumentParts} as &$tempDocumentPart) { is located at. It is not in the develop's branch TemplateProcessor.php file.
In my corrections at #1162 there is one place where two foreach'es with &$item go one after another. So it is definitely a problem.

@FBnil
Copy link

FBnil commented Oct 20, 2017

I mean that PHP as a language is not THAT broken, this works perfectly fine:

<?php

$Q = [2,4,6];
$R = [10,20,30];

foreach($Q as &$item){
        $item++;
}

foreach($R as &$item){
        $item++;
}

var_dump($Q);
var_dump($R);

Prints [3,5,7] and [11,21,31], as expected

Granted, at the end of both and within the same function, you should never use item, because:

$item = 100;
var_dump($R);

(Prints [11,21,100])

But in our case, we are in a function, and when the function ends, the variable goes out of scope and all is fine:

<?php

$Q = [2,4,6];
$R = [10,20,30];

function addOne(&$Arr){
        foreach($Arr as &$item){
                $item++;
        }
}

addOne($Q);
addOne($R);
$item = 100;

var_dump($Q);
var_dump($R);

@FBnil
Copy link

FBnil commented Oct 20, 2017

FYI: Existing bugs in TemplateProcessor

I've ironed them out in my branch.

@troosan troosan added this to the v0.16.0 milestone Dec 26, 2018
@troosan troosan merged commit 6cf10b4 into PHPOffice:develop Dec 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants