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

Make it PHP 8.2 compatible #105

Closed
kapilpaul opened this issue Oct 10, 2023 · 5 comments
Closed

Make it PHP 8.2 compatible #105

kapilpaul opened this issue Oct 10, 2023 · 5 comments
Labels
bug This issue is a bug. investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 This is a standard priority issue

Comments

@kapilpaul
Copy link

kapilpaul commented Oct 10, 2023

Describe the bug

Hi,

I am using this package in one of my Projects. after running the PHPCS for 8.2 found the below issue in this line.

FILE: vendor/aws/aws-crt-php/src/AWS/CRT/NativeResource.php
------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------
 40 | ERROR | "$this" can no longer be unset since PHP 7.1.
(PHPCompatibility.Variables.ForbiddenThisUseContexts.Unset)
------------------------------------------------------------------------------------------------------------------------

https://github.com/awslabs/aws-crt-php/blob/2f1dc7b7eda080498be96a4a6d683a41583030e9/src/AWS/CRT/Internal/Encoding.php#L27C10-L27C35

FILE: vendor/aws/aws-crt-php/src/AWS/CRT/Internal/Encoding.php
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 27 | WARNING | Using ${var} in strings is deprecated since PHP 8.2, use {$var} instead. Found: ${len}
(PHPCompatibility.TextStrings.RemovedDollarBraceStringEmbeds.DeprecatedVariableSyntax)
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

can you please fix it to make it compatible?

Expected Behavior

The same behavior as it is now.

Current Behavior

There is no issue in behavior right now. Just need to make the code change to make it PHP 8.1 compatible.

Reproduction Steps

phpcs -s -d memory_limit=2048M --standard=PHPCompatibility --basepath=./ --runtime-set testVersion 8.1- -p --extensions=php file_path_here

Possible Solution

No response

Additional Information/Context

No response

aws-crt-php version used

1.2.2

php version used

8.2.10

Operating System and version

MAC OS

@kapilpaul kapilpaul added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 10, 2023
@jmklix jmklix self-assigned this Oct 10, 2023
@jmklix jmklix added investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Oct 10, 2023
@kapilpaul kapilpaul changed the title Make it PHP 8.1 compatible Make it PHP 8.2 compatible Oct 10, 2023
@michaelbragg
Copy link

Can confirm that we are seeing the '"$this" can no longer be unset since PHP 7.1.' Error when running PHP Compatibility on PHP 8.0, 8.1, 8.2.

@jmklix
Copy link
Member

jmklix commented Oct 18, 2023

We are looking into making a fix for this

@mdio
Copy link
Contributor

mdio commented Nov 6, 2023

I've created PR #108 to fix the issue with ${len} and confirmed that the $this error is a false positive.

The code is

unset(self::$resources[spl_object_hash($this)]);

which still works in PHP 8.2 as it's not unsetting $this itself but just passing it as a parameter to spl_object_hash() to compute the key for self::$resources to unset.

@waahm7
Copy link
Contributor

waahm7 commented Nov 8, 2023

This should be fixed in https://github.com/awslabs/aws-crt-php/releases/tag/v1.2.4. @mdio Thank you for investigating and the PR.

@waahm7 waahm7 closed this as completed Nov 8, 2023
@mdio
Copy link
Contributor

mdio commented Nov 8, 2023

Thank you for the quick review and release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

5 participants