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

PHP 8.1 deprecations #919

Merged
merged 6 commits into from
Oct 28, 2021
Merged

Conversation

arokettu
Copy link
Contributor

@arokettu
Copy link
Contributor Author

I addressed doctrine test crashes separately: #920

@arokettu
Copy link
Contributor Author

I'm not sure what to do with this one:

1) Payum\Core\Tests\Security\SensitiveValueTest::shouldNotSerializeValue
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'N;'
+'O:34:"Payum\Core\Security\SensitiveValue":0:{}'

You can't serialize to null in 8.1+

@pierredup pierredup added this to the 1.6.3 milestone Oct 15, 2021
@arokettu
Copy link
Contributor Author

Rebased the PR. Serialization seems to be the only problem

@pierredup
Copy link
Member

@arokettu I think you can wrap that test just in an if and check the expected value based on the PHP version

if (PHP_VERSION_ID >= 70400) {
    $this->assertEquals('O:34:"Payum\Core\Security\SensitiveValue":0:{}', $serializedValue);
} else {
    $this->assertEquals('N;', $serializedValue);
}

@arokettu
Copy link
Contributor Author

@pierredup I'm more concerned whether it is not acceptable / acceptable but bc break / whatever other problem there might be

Fixing tests is easy

@pierredup
Copy link
Member

pierredup commented Oct 25, 2021

@arokettu The SensitiveValue class is not supposed to be serializable, which is why there's a test to ensure this, and also why the serialize methods are added to block the value from being serialized.

IMHO if someone depends on this class being able to serialize to null, they are depending on undocumented behaviour that's not guaranteed, so I don't consider this change as a BC break.

@arokettu
Copy link
Contributor Author

Ok, thank you

@arokettu
Copy link
Contributor Author

Remaining deprecations:

  2x: utf8_encode(): Passing null to parameter #1 ($string) of type string is deprecated
    2x in GetAddressesActionTest::shouldCallKlarnaGetAddresses from Payum\Klarna\Invoice\Tests\Action\Api

  2x: utf8_decode(): Passing null to parameter #1 ($string) of type string is deprecated
    1x in PopulateKlarnaFromDetailsActionTest::shouldNotFaileIfEmptyDetailsGiven from Payum\Klarna\Invoice\Tests\Action\Api
    1x in PopulateKlarnaFromDetailsActionTest::shouldCorrectlyPutPartialArticles from Payum\Klarna\Invoice\Tests\Action\Api

  1x: htmlspecialchars(): Passing null to parameter #1 ($string) of type string is deprecated
    1x in CaptureOffsiteActionTest::shouldRedirectToBe2billSiteIfExecCodeNotPresentInQuery from Payum\Be2Bill\Tests\Action

I couldn't trace the last one, may be an external one.

I don't know how to fix Klarna because I don't know what values it expects, I never used it. But anyway, Klarna's lib is not PHP 8.1 ready itself so the fix can be postponed.

@arokettu
Copy link
Contributor Author

If that's ok I think it's ready. At least the Core is now PHP 8.1 ready

@pierredup
Copy link
Member

Thank you @arokettu

@pierredup pierredup merged commit b24e043 into Payum:master Oct 28, 2021
@arokettu arokettu deleted the php-81-compatibility branch October 29, 2021 00:04
pierredup pushed a commit to aliriaz-stripe/Payum that referenced this pull request Dec 3, 2021
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.

None yet

2 participants