Skip to content

Fix optionnal parameters declaration (PHP8) #293

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 9, 2020
Merged

Fix optionnal parameters declaration (PHP8) #293

merged 3 commits into from
Dec 9, 2020

Conversation

cedric-anne
Copy link
Contributor

Some deprecation errors are trigerred when exporting a PDF in PHP 8. If these errors are displayed, it prevents sending valid PDF headers/contents to the browser.

Even if these params are always set in internal use of fixed methods, I kept their optionnal state as they are declared in public methods.

PHP Deprecated function (8192): Required parameter $currentfont follows optional parameter $isunicode in ./vendor/tecnickcom/tcpdf/include/tcpdf_fonts.php at line 1998
PHP Deprecated function (8192): Required parameter $currentfont follows optional parameter $isunicode in ./vendor/tecnickcom/tcpdf/include/tcpdf_fonts.php at line 2024
PHP Deprecated function (8192): Required parameter $currentfont follows optional parameter $setbom in ./vendor/tecnickcom/tcpdf/include/tcpdf_fonts.php at line 2040
PHP Deprecated function (8192): Required parameter $currentfont follows optional parameter $setbom in ./vendor/tecnickcom/tcpdf/include/tcpdf_fonts.php at line 2060
PHP Deprecated function (8192): Required parameter $currentfont follows optional parameter $str in ./vendor/tecnickcom/tcpdf/include/tcpdf_fonts.php at line 2077
PHP Deprecated function (8192): Required parameter $currentfont follows optional parameter $str in ./vendor/tecnickcom/tcpdf/include/tcpdf_fonts.php at line 2093
PHP Deprecated function (8192): Required parameter $tagvspaces follows optional parameter $default_css in ./vendor/tecnickcom/tcpdf/include/tcpdf_static.php at line 1139
PHP Deprecated function (8192): Required parameter $k follows optional parameter $points in ./vendor/tecnickcom/tcpdf/include/tcpdf_static.php at line 2510

@CLAassistant
Copy link

CLAassistant commented Oct 14, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@brainfoolong
Copy link
Contributor

Hi, this errors have been already fixed with another pull request -> #265

@cedric-anne
Copy link
Contributor Author

Hi, this errors have been already fixed with another pull request -> #265

Hi,

Indeed, but I did not fix it the same way (I kept the fact that params are optionnal), and my PR focus only on this point, hoping that it will fasten its review/merge.

Regards

@brainfoolong
Copy link
Contributor

I see. Just a note: You and my fix is effectively the same. As a required parameter after a optional is "invalid" since ever, when you call it. Only declaration checks has been changed in PHP8. So, a code using any of those functions has thrown an error in every php version, when you not passing the required parameters.

So i prefer to change declaration to make it clear that no parameter is optional, as it never was. But it's maybe just my oppinion.

You can see the behaviour here: https://3v4l.org/NvcVl

And, i really hope that the maintainer of this projects finally do some merges in this abonded project. But that's another story... :)

@nicolaasuni nicolaasuni merged commit 89f9e5f into tecnickcom:main Dec 9, 2020
This was referenced Dec 9, 2020
Closed
Closed
@cedric-anne cedric-anne deleted the fix/php8 branch December 9, 2020 08:13
@cedric-anne
Copy link
Contributor Author

Hi @nicolaasuni ,

Thanks for the merge. Do you already planned a release with latests fixes ?

Regards

@sgiehl sgiehl mentioned this pull request Dec 10, 2020
Merged
9 tasks
@williamdes
Copy link
Contributor

@ricoBolo06 you should test this fix, it was merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolaasuni nicolaasuni nicolaasuni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cedric-anne @CLAassistant @brainfoolong @williamdes @nicolaasuni