Samples-as-a-tests by seba-aln · Pull Request #119 · pubnub/php

seba-aln · 2025-06-24T13:12:06Z

This adds a lot of samples. Improves testing and coverage

pubnub-ops-terraform · 2025-06-24T13:12:26Z

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

marcin-cebo · 2025-06-24T16:16:57Z

examples/AccessManager.php

+
+// snippet.uuid_metadata_operations
+try {
+    $result = $user->setUUIDMetadata()


I other place you used:
$admin->setUuidMetadata()
Is this desired ?

PHP's function names are case insensitive. this though should be written with uppercase uuid

marcin-cebo · 2025-06-24T16:18:35Z

examples/AccessManager.php

+}
+
+// Helper function to safely execute operations (deprecated - using direct try/catch now)
+function safeTest($callable, $description)


Is this method needed? Where it is used?

nope. probably leftover from copypaste

marcin-cebo · 2025-06-24T16:23:15Z

tests/Examples/AppContextTest.php

+
+class AppContextTest extends TestCase
+{
+    public function testExamples(): void


Could this name be more descriptive?

It's a test for examples. how much descriptive a single method name in pretty much self explanatory class should be?

Maybe testAppContextCodeSnippets ?

marcin-cebo · 2025-06-24T16:26:11Z

examples/AppContext.php

+$subscribeKey = getenv('SUBSCRIBE_KEY') ?: 'demo';
+
+$config = new PNConfiguration();
+$config->setSubscribeKey(getenv('SUBSCRIBE_KEY', 'demo'));


Why not to pass $publishKey variable here?

marcin-cebo · 2025-06-24T16:28:31Z

examples/AppContext.php

+        ->profileUrl($user['profileUrl'])
+        ->custom($user['custom'])
+        ->sync();
+    assert($setUserMetadataResult->getId());


What do you think about: "By default, PHP compiles out assert() calls unless you enable zend.assertions=1 and assert.exception=1. In a typical CLI script they won’t even run, so you’ll never know if one failed."
?

From what I've found in docs and on php.watch symbols history the default for zend.assertions is 1 so they are by default enabled.

marcin-cebo · 2025-06-25T07:14:50Z

src/PubNub/Crypto/AesCbcCryptor.php

+        return random_bytes(static::IV_LENGTH);
    }

    public function getCipherKey($cipherKey = null): string


Shouldn't be type-hinted?

marcin-cebo · 2025-06-25T07:16:41Z

src/PubNub/Crypto/AesCbcCryptor.php

@@ -41,17 +41,17 @@ public function encrypt(string $text, ?string $cipherKey = null): CryptoPayload
    {


In getSecret method is this required to check:
!is_null($cipherKey)
if method signature enforce cipherKey being not null?

Probably redundant, but not the scope of this task.

marcin-cebo · 2025-06-25T07:22:08Z

src/PubNub/CryptoModule.php

+        return new static(
            [
                LegacyCryptor::CRYPTOR_ID => new LegacyCryptor($cipherKey, $useRandomIV),
                AesCbcCryptor::CRYPTOR_ID => new AesCbcCryptor($cipherKey),


Instead of:
aesCbcCryptor::CRYPTOR_ID
shouldn't be:
AesCbcCryptor::CRYPTOR_ID
?

marcin-cebo · 2025-06-25T07:30:11Z

src/PubNub/Endpoints/Objects/Membership/GetMemberships.php

@@ -193,7 +193,7 @@ protected function customParams()
                }


Is this ok for buildData method to return null when signature declare return type as string ?

the type in comment is just a hint for compatibility with parent IIRC

marcin-cebo · 2025-06-25T09:43:05Z

tests/integrational/FilesTest.php

@@ -139,4 +171,361 @@ public function testThrowErrorOnNoFileFound(): void
        $this->expectException(PubNubServerException::class);


In testThrowErrorOnMalformedResponse
there is
$pubnub->setClient($client);
and
$this->pubnub->setClient($client);

Are those two needed?

$this->pubnub->setClient($client); this one was not needed

Samples-as-a-tests

a6057ec

seba-aln requested review from Xavrax and marcin-cebo as code owners June 24, 2025 13:12

Sebastian Molenda added 3 commits June 24, 2025 15:18

Linter +

9e54e78

Update workflow

e732c0f

Cleanup

b6ae34f

marcin-cebo approved these changes Jun 25, 2025

View reviewed changes

Sebastian Molenda added 3 commits June 26, 2025 12:34

Review fixes + skip await for token revokation

8ae74ae

fixes to test run

963de06

missing codacy config

bc953d5

seba-aln merged commit 2cd05ff into master Jun 26, 2025
12 checks passed

seba-aln deleted the tests/add-samples-as-a-test branch June 26, 2025 11:58

		@@ -41,17 +41,17 @@ public function encrypt(string $text, ?string $cipherKey = null): CryptoPayload
		{

		@@ -139,4 +171,361 @@ public function testThrowErrorOnNoFileFound(): void
		$this->expectException(PubNubServerException::class);

Conversation

seba-aln commented Jun 24, 2025

Uh oh!

pubnub-ops-terraform commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎉 Snyk checks have passed. No issues have been found so far.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pubnub-ops-terraform commented Jun 24, 2025 •

edited

Loading