Remove #[SensitiveParameter] attributes for PHP 8.1 support

[webns]

In this PR, I've reverted commit that adds SensitiveParameter attribute to properties because we cannot use it with PHP 8.1. Despite the fact that PHP itself works correctly with this attributes, the issue arises when using reflection, which is used in the cuyz/valinor package.

To prove that we could run integration tests with PHP 8.1 and receive errors like that

Error example

1) Kreait\Firebase\Tests\Integration\AppCheckTest ReflectionException: Class "SensitiveParameter" does not exist

/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Utility/Reflection/Reflection.php:60
/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Definition/Repository/Reflection/ReflectionAttributesRepository.php:82
/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Definition/Repository/Reflection/ReflectionAttributesRepository.php:40
/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Definition/Repository/Reflection/ReflectionPropertyDefinitionBuilder.php:51
/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Definition/Repository/Reflection/ReflectionClassDefinitionRepository.php:143
/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Definition/Repository/Reflection/ReflectionClassDefinitionRepository.php:91
/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Definition/Repository/Cache/InMemoryClassDefinitionRepository.php:23
/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Mapper/Tree/Builder/ValueConverterNodeBuilder.php:44
/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Mapper/Tree/Shell.php:73
/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Mapper/Tree/RootNodeBuilder.php:46
/Users/ns/Documents/Playground/firebase-php/vendor/cuyz/valinor/src/Mapper/TypeTreeMapper.php:32
/Users/ns/Documents/Playground/firebase-php/src/Firebase/Valinor/Mapper.php:58
/Users/ns/Documents/Playground/firebase-php/src/Firebase/Factory.php:762
/Users/ns/Documents/Playground/firebase-php/src/Firebase/Factory.php:159
/Users/ns/Documents/Playground/firebase-php/tests/IntegrationTestCase.php:56

I've changed the integration test workflow so that they run on all PHP versions, but I'm not sure whether it was originally set up that way for a specific reason. So we can revert to running them on a single version if needed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.70%. Comparing base (2de5c2c) to head (0792388).
⚠️ Report is 1 commits behind head on 7.x.

Additional details and impacted files

@@             Coverage Diff              @@
##                7.x    #1034      +/-   ##
============================================
+ Coverage     68.90%   72.70%   +3.80%     
  Complexity     1468     1468              
============================================
  Files           145      145              
  Lines          4177     4169       -8     
============================================
+ Hits           2878     3031     +153     
+ Misses         1299     1138     -161     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jeromegamez]

Hey, and thanks for bringing this to my attention! I haven't encountered this before, and I'm curious what the integration tests will say.

In the meantime could you please limit your changes to the removal of the #[SensitiveParameter] lines?

[webns]

Should I remove changes from cs-fixer config that was introduced in the same commit that #[SensitiveParameter] was added or I can revert them as well?

[jeromegamez]

Should I remove changes from cs-fixer config that was introduced in the same commit that #[SensitiveParameter] was added or I can revert them as well?

Just the changes in the ServiceAccount and AppCheckTokenGenerator should suffice, just to avoid potential code style changes in other places going forward.

I'm really curious why this doesn't work - the errors in the job run are just™ the brittle tests as usual or unrelated, and the tests themselves use the valinor'ed service accounts.

[jeromegamez]

I've changed the integration test workflow so that they run on all PHP versions, but I'm not sure whether it was originally set up that way for a specific reason. So we can revert to running them on a single version if needed.

Sorry I didn't see this earlier - the integration tests run on just one PHP version to save time and avoid conflicts when multiple tests would run on the same Firebase project.

But that explains of course why the problem doesn't occur in the integration test, if the PHP 8.1 version is the only one affected.

I do think it should be able to cover this particular problem with unit tests, though. If you don't beat me to it, I can attempt to add one later.

[webns]

Thank you for your blazingly fast responses 👍 I've thought about writing unit test that covers that problem by myself but I don't know where is good place to write that test

It would be nice if you can add them. I've noticed that when we use PHP 8.1 and call getAttributes method on a Reflection class we see SensitiveParameter in a list, but with PHP 8.2+ we don't have that in returned value

Simple example

<?php

#[\Attribute]
class SomeOtherAttribute{}

class Example {
    public function __construct(
        #[\SomeOtherAttribute]
        #[\SensitiveParameter]
        protected string $value,
    ) {
    }
}

$reflClass = new \ReflectionClass(Example::class);

$properties = $reflClass->getProperties();

foreach ($properties as $reflProperty) {
    $attributes = $reflProperty->getAttributes();

    foreach ($attributes as $reflAttribute) {
        echo implode('::', [$reflClass->getName(), $reflProperty->getName()]) . " => " . $reflAttribute->getName() . PHP_EOL;
    }
}

Result for PHP 8.2+:

Example::value => SomeOtherAttribute

Result for PHP 8.1:

Example::value => SomeOtherAttribute
Example::value => SensitiveParameter

[jeromegamez]

Sorry for being later than promised: I was able to confirm the issue locally with a test that failed as you described. Pushing the test as a new commit to your branch didn't work (permission denied), but if you'd be okay with that, I would create a commit directly to the main branch and make sure it's attributed to you, would that be alright with you?

[webns]

Sure, that's fine for me

[jeromegamez]

I went ahead and merged the PR as is and added my test to assert this won't break going forward in 80f41cf - this kept your contribution fully intact, which is more important than having the test and the fixed bundled in one commit.

This has now been released as 7.24.0. Thank you very much for reporting and fixing this!