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

[BUGFIX] Remember current template id #100

Merged
merged 1 commit into from
Dec 1, 2018
Merged

Conversation

liayn
Copy link
Contributor

@liayn liayn commented Nov 30, 2018

Caveat: A template with multiple pages will only use the first page of it

Fixes #99

@liayn
Copy link
Contributor Author

liayn commented Nov 30, 2018

A really dirty workaround

}
}

public function setCurrentTemplateId(int $templateId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not use type hints for primitive data types as we still support PHP 5.6.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we still support PHP 5.6

I guess only for December, right? ;-)

Copy link
Member

@maechler maechler Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are thinking about dropping support for PHP < 7 next year, yes. But if we do so, we would also like to add type hints to all functions and that would be considered a breaking change. Thus I am not yet sure how soon we will make this transition.

/**
* @var int
*/
protected $currentTemplateId = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this variable already declared in FpdfTplTrait?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it is not. PHPStorm would have told me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see..a different trait is included, sry!

@maechler
Copy link
Member

Thanks for the pull request!

Caveat: A template with multiple pages will only use the first page of it

If I get this correctly, that means that it works just as before? If you use importPage a second time on a different page that should work, or am I wrong?
I do not think that multiple pages would have been imported automatically anyways, although I have to admit I never tested this use case.

Ceveat: A template with multiple pages will only use the first page of it

Fixes bithost-gmbh#99
@liayn
Copy link
Contributor Author

liayn commented Nov 30, 2018

Updated the patch to be really working now and without typehints.

@maechler
Copy link
Member

Thanks, I will try to test and release this as soon as possible!

@liayn
Copy link
Contributor Author

liayn commented Nov 30, 2018

This is a workaround. I'm not sure if this is the right thing to release actually. But I don't know how the inner stuff in fpdf works, so I guess you know better.

@maechler maechler changed the base branch from master to develop December 1, 2018 16:14
@maechler maechler changed the base branch from develop to master December 1, 2018 16:18
@maechler maechler changed the base branch from master to develop December 1, 2018 16:18
@maechler maechler merged commit a0d453e into bithost-gmbh:develop Dec 1, 2018
@liayn liayn deleted the fix-99 branch December 6, 2018 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants