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

Fixes for PHP 8.2 in tcpdf_fonts.php - fixes #632 #633

Merged
merged 5 commits into from
Sep 6, 2023

Conversation

littlepackage
Copy link
Contributor

Fixes "use of "self" in callables is deprecated" warning is arising from tcpdf_fonts.php when using PHP >= 8.2

Fixes "use of "self" in callables is deprecated" warning is arising from tcpdf_fonts.php when using PHP >= 8.2
@CLAassistant
Copy link

CLAassistant commented Aug 1, 2023

CLA assistant check
All committers have signed the CLA.

@littlepackage littlepackage changed the title Update tcpdf_fonts.php Fixes for PHP 8.2 in tcpdf_fonts.php Aug 1, 2023
Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Do you know if it breaks old PHP versions ?
We still support 5.4..

@littlepackage
Copy link
Contributor Author

PHP 5.4... Wow. This should work down to PHP 5.5.

I guess I'd have to write some sort of phpversion() check. Would that be OK?

@williamdes
Copy link
Contributor

PHP 5.4... Wow. This should work down to PHP 5.5.

I guess I'd have to write some sort of phpversion() check. Would that be OK?

I checked composer.json it's even worse: 5.3
I think that would be okay, keep your patch for all versions and do an if for old versions to have the old version of the code

PHP 8.2 "use of "self" in callables is deprecated" yet some ways of fixing this breaks for PHP 5.3. This approach works, tested PHP 5.3.29 - 8.2.0
@littlepackage
Copy link
Contributor Author

5.3! Gasp! 😋 I was able to test and confirm another approach (committed to #633) works using PHP 5.3.29 through 8.2.

Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Awesome!!!
You removed one space before the argument, can you add it back?

Spaces added back in before arguments
@littlepackage
Copy link
Contributor Author

Spaces done, I think?

Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Yup look great

@williamdes
Copy link
Contributor

Please add Fixes: #632 to your PR description so it links it and closes it on merge

@littlepackage littlepackage changed the title Fixes for PHP 8.2 in tcpdf_fonts.php Fixes for PHP 8.2 in tcpdf_fonts.php - fixes #632 Aug 2, 2023
@littlepackage
Copy link
Contributor Author

littlepackage commented Aug 3, 2023

Sorry - I've made a mistake. This fix doesn't work, and probably won't work without a phpversion() conditional and an include() to move code (self::class) which WILL break 5.3 to another file. PHP 5.5 is as low as the "easy fix" goes. Awkward! Hopefully other ideas roll in.

@littlepackage littlepackage changed the title Fixes for PHP 8.2 in tcpdf_fonts.php - fixes #632 Fixes for PHP 8.2 in tcpdf_fonts.php Aug 3, 2023
@williamdes
Copy link
Contributor

Sorry - I've made a mistake. This fix doesn't work, and probably won't work without a phpversion() conditional and an include() to move code (self::class) which WILL break 5.3 to another file. PHP 5.5 is as low as the "easy fix" goes. Awkward! Hopefully other ideas roll in.

Is there only 5.3 that is broken?

@littlepackage
Copy link
Contributor Author

This fix starts working with PHP 5.5.

It seems it'd be a bit of an ordeal to make it work 5.3-8.2 because of these callables. Do-able but not pretty!

@littlepackage
Copy link
Contributor Author

littlepackage commented Aug 3, 2023

Interestingly, PHP 5.3 EOL was almost exactly ten years ago: 14 Aug 2014 😳

Edit: Today felt like 2024.

@williamdes
Copy link
Contributor

Interestingly, PHP 5.3 EOL was almost exactly ten years ago: 14 Aug 2014 😳

😳😨

Can it be a variable and replaced if version compare === old?

@littlepackage
Copy link
Contributor Author

5.3 parses everything before run and when it sees self::class, it breaks. ☹️ That's why I was mentioning using phpversion() to check for >8.2 and including that in a separate file, maybe.

@williamdes
Copy link
Contributor

self::class

If I am not mistaken self::class is just a string in the end, can you hard code the string ?
I would bet it's just \CLASS_NAME

@littlepackage
Copy link
Contributor Author

littlepackage commented Aug 3, 2023

self::class is a string but 5.3 sees that :: in the file and 😵

Something like this might work:

$selfclass = get_called_class();
if ( $isunicode ) {
    return array_map($selfclass . '::unichrUnicode', $ta );
}
return array_map($selfclass . '::unichrASCII', $ta );

I'm testing now.

@williamdes
Copy link
Contributor

self::class is a string but 5.3 sees that :: in the file and

I meant that is you var_dump(self::class) you will get it's final form ;)

@littlepackage
Copy link
Contributor Author

Sure, TCPDF_Fonts. Have you tried what you're suggesting?

@williamdes
Copy link
Contributor

Sure, TCPDF_Fonts. Have you tried what you're suggesting?

Hmm, sorry I got confused in my brain

So with a version compare it can be changed between TCPDF_FONTS and self.
Should be okay for the linter and the code execution, right ?

@littlepackage
Copy link
Contributor Author

littlepackage commented Aug 3, 2023

Because writing self::class anywhere in a file called by TCPDF (such as tcpdf_fonts.php) will break PHP 5.3. Similar to how using array() notation [] in a PHP 5.3 file will break it. The other solution I've mentioned with include() is much more complicated, and involves checking for PHP > 8.2, and including a separate file conditionally which isn't seen by 5.3 at all.

if ( version_compare( phpversion(), '5.3', '>' ) ) {
    if ( ! in_array( 'nuts', [ 'nutty', 'array' ] ) ) {
        // Doesn't even get here due to syntax error involving use of []
	die( 'Well, nuts' );
    }
}

Using get_called_class() (like I did above) is the simplest solution I've found that works. I'll do some more testing here to make sure.

@littlepackage
Copy link
Contributor Author

I think when I've begun coding about nuts I've lost my mind. 🥜🧠 Will test this a bit more and update the PR!

@williamdes
Copy link
Contributor

Because writing self::class anywhere in a file called by TCPDF (such as tcpdf_fonts.php) will break PHP 5.3. Similar to how using array() notation [] in a PHP 5.3 file will break it. The other solution I've mentioned with include() is much more complicated, and involves checking for PHP > 8.2, and including a separate file conditionally which isn't seen by 5.3 at all.

if ( version_compare( phpversion(), '5.3', '>' ) ) {
    if ( in_array( 'nuts', [ 'nutty', 'array' ] ) ) {
        // Doesn't even get here due to syntax error
	die( 'Well, nuts' );
    }
}

Using get_called_class() (like I did above) is the simplest solution I've found that works. I'll do some more testing here to make sure.

I agree, that said you do not need to write self::class but only it's final form and it will work great too
that said I like your get_called_class solution, it may solve this nicely without too much lines
it it maybe works for all php versions

@littlepackage
Copy link
Contributor Author

Using something like 'TCPDF_FONTS::uniord' would work in older versions of PHP but not newer versions 🤷‍♀️ It stopped working with PHP 8. I'm glad you like the get_called_class() solution well enough 😊

Maneuvers compatibility of callables inside array_map() between PHP 5.3 and 8.2 - tested.
@littlepackage littlepackage changed the title Fixes for PHP 8.2 in tcpdf_fonts.php Fixes for PHP 8.2 in tcpdf_fonts.php - fixes #632 Aug 3, 2023
Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Thank you so much for your hard work !

@littlepackage
Copy link
Contributor Author

You are welcome!

I've been using this library since well... PHP 5.3, so it's the least I can do to finally jump in on Github and help. 👵🏼

@williamdes
Copy link
Contributor

You are welcome!

I've been using this library since well... PHP 5.3, so it's the least I can do to finally jump in on Github and help. 👵🏼

So cool!
I will gladly welcome you on other open source projects I contribute to

@nicolaasuni nicolaasuni merged commit 35e4147 into tecnickcom:main Sep 6, 2023
0 of 38 checks passed
@jmv69800
Copy link

Hi
I'm very new in TCPDF.
Also very new in GitHub (first use, in fact). Please, accept my appolgies if I don't use GitHub correctly.

I tested the proposed fixe under PHP 8.2.9 x86 (in preparation to use TCPDF in PHP 8.3)
... and it seems not working.

I've reported the 3 new lines in tcpdf_fonts.php, and removed the old 3 existing ones, but I obtain that

Deprecated: Optional parameter $isunicode declared before required parameter $currentfont is implicitly treated as a required parameter in .\tcpdf\include\tcpdf_fonts.php on line 2000, and 2027, 2043, etc...

Also $setbom, $forcertl, $str, $default_css, $tagvs, etc... deprecated (perhaps due to the first deprecated)

Is there something I've done wrong ?

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.

5 participants