awslabs / aws-crt-php

Apache License 2.0
322 stars 13 forks source link

Make it PHP 8.2 compatible #105

Closed kapilpaul closed 8 months ago

kapilpaul commented 8 months ago

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

michaelbragg commented 8 months ago

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 commented 8 months ago

We are looking into making a fix for this

mdio commented 8 months ago

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 commented 8 months ago

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.

mdio commented 7 months ago

Thank you for the quick review and release!