Refactor/replace transport with guzzle by seba-aln · Pull Request #116 · pubnub/php

seba-aln · 2025-02-25T15:12:55Z

refactor: Replace dependency from Requests to Guzzle.

Replace dependency from Requests to GuzzleHTTP to allow communication over HTTP/2. This is potentially breaking change because removes the old way to set up custom transport with setting the client dependency. Read more in the documentation (migration guide available)

marcin-cebo · 2025-03-12T17:48:33Z

examples/Membersips/channel_members.php

+        ->setCustom(["a" => "b"])
+        ->setStatus("status")
+        ->setType("type"),
+    (new PNChannelMember('uuid1'))


Is this ok to add PNChannelMember with the same uuid1 as above?

marcin-cebo · 2025-03-12T17:52:10Z

examples/simple_partial_updates.php

+$additionalMetadata = ["Months" => "Jan-May"];
+
+// Merge additional metadata
+$updatedCustomMetadata = array_merge($custom, $additionalMetadata);


Wouldn't
$updatedCustomMetadata = $custom + $additionalMetadata;
be better not ot overwrite existing keys?

that only depends on the goal you want to achieve. over here I show how to update the metadata so array_merge seems better. I guess if you'd change some fields, do a union and after update you get the "old" values then you'd think something is not right.

marcin-cebo · 2025-03-12T17:53:11Z

examples/simple_partial_updates.php

+$updatedCustomMetadata = array_merge($custom, $additionalMetadata);
+
+// Update the channel metadata
+$updatedMetadata = $pubnub->setChannelMetadata()


What would happened when exception is thrown. Shouldn't we add try/catch block here?

it's just a simple example. I didn't give it a second thought TBH

marcin-cebo · 2025-03-12T17:55:03Z

examples/simple_partial_updates.php

+// Set initial channel metadata
+$pubnub->setChannelMetadata()
+    ->channel($channel)
+    ->meta(["custom" => $initialCustom])


is property called meta or custom ?

the old implementation had just the meta property where one had to create a valid structure on it's own. Right now there are 'convenience methods' to set custom name and other fields without relying on deep arcane knowledge of the schema :)

marcin-cebo · 2025-03-12T17:57:10Z

src/PubNub/Models/Consumer/FileSharing/PNGetFileDownloadURLResult.php

+    public function __construct(array $response)
    {
-        $this->fileUrl = $result->headers->getAll()['location'][0];
+        $this->fileUrl = $response['Location'][0];


What about:
$this->fileUrl = isset($response['Location'][0]) ? $response['Location'][0] : '';
or
$this->fileUrl = $response['Location'][0] ?? '';

You Sir are absolutely right. This part could use some hardening i guess

marcin-cebo · 2025-03-12T17:58:09Z

src/PubNub/Models/Consumer/FileSharing/PNGetFileDownloadURLResult.php

@@ -6,9 +6,14 @@ class PNGetFileDownloadURLResult
 {
    protected string $fileUrl;


what about:
protected ?string $fileUrl = null;

If Location[0] is not set, the property will be assigned an empty string (''), but it should ideally be null to indicate an error ?

You Sir are absolutely right. This part could use some hardening i guess

marcin-cebo · 2025-03-12T18:00:59Z

src/PubNub/Models/Consumer/Objects/Membership/PNMembershipIncludes.php

        $this->mapping = array_merge($this->mapping, [
-            'channel' => 'channel',
-            'channelId' => 'channel.id',
+            'channel' => 'channel.id',


FYI in kotlin SDK there is:
MembershipInclude(
includeCustom = true,
includeStatus = true,
includeType = true,
includeChannel = true,
includeChannelCustom = true,
includeChannelType = true,
includeChannelStatus = true,
includeTotalCount = true
),

You mean that it misses custom, status, type and total count ? They are present in the PNIncludes class.
Or it's about not having Include prefix? I thought it would be redundant as the entire object is called PNMembership*Includes*

Here:
https://www.pubnub.com/docs/sdks/rest-api/get-membership-metadata
in section Query Parameters#include there are:
Possible values: [custom, type, status, channel, channel.custom, channel.status, channel.type]

Does you code allows specifying all those properties?

marcin-cebo · 2025-03-12T18:07:51Z

src/PubNub/PubNub.php

+     * @param ClientInterface $httpClient
+     * @return void
+     */
+    public function setClient(ClientInterface $httpClient): void


If the client or request factory is changed mid-execution, it could introduce inconsistent behavior.

what do you mean "mid execution"? this SDK is not yet asynchronous (and not planned to be async or non-blocking) so there should not be a thing that would overwrite client "mid-execution"

marcin-cebo · 2025-03-12T18:07:55Z

src/PubNub/PubNub.php

+     * @param RequestFactoryInterface $requestFactory
+     * @return void
+     */
+    public function setRequestFactory(RequestFactoryInterface $requestFactory): void


If the client or request factory is changed mid-execution, it could introduce inconsistent behavior.

same ... to change anything you have to wait until nothing else is running

marcin-cebo · 2025-03-17T15:07:54Z

src/PubNub/Models/Consumer/Objects/Membership/PNMembershipIncludes.php

        $this->mapping = array_merge($this->mapping, [
-            'channel' => 'channel',
-            'channelId' => 'channel.id',
+            'channel' => 'channel.id',


Here:
https://www.pubnub.com/docs/sdks/rest-api/get-membership-metadata
in section Query Parameters#include there are:
Possible values: [custom, type, status, channel, channel.custom, channel.status, channel.type]

Does you code allows specifying all those properties?

marcin-cebo · 2025-03-17T15:09:18Z

src/PubNub/PubNub.php

+     * @param ClientInterface $httpClient
+     * @return void
+     */
+    public function setClient(ClientInterface $httpClient): void


marcin-cebo · 2025-03-17T15:09:29Z

src/PubNub/PubNub.php

+     * @param RequestFactoryInterface $requestFactory
+     * @return void
+     */
+    public function setRequestFactory(RequestFactoryInterface $requestFactory): void


marcin-cebo · 2025-03-17T15:10:18Z

examples/simple_partial_updates.php

+$additionalMetadata = ["Months" => "Jan-May"];
+
+// Merge additional metadata
+$updatedCustomMetadata = array_merge($custom, $additionalMetadata);


marcin-cebo · 2025-03-17T15:10:29Z

examples/simple_partial_updates.php

+$updatedCustomMetadata = array_merge($custom, $additionalMetadata);
+
+// Update the channel metadata
+$updatedMetadata = $pubnub->setChannelMetadata()


marcin-cebo · 2025-03-17T15:10:50Z

examples/simple_partial_updates.php

+// Set initial channel metadata
+$pubnub->setChannelMetadata()
+    ->channel($channel)
+    ->meta(["custom" => $initialCustom])


seba-aln · 2025-03-19T13:31:16Z

@pubnub-release-bot release as 8.0.0

CHANGELOG.md

pubnub-release-bot · 2025-03-19T13:41:33Z

🚀 Release successfully completed 🚀

Sebastian Molenda added 13 commits February 20, 2025 18:20

Guzzle 1 step

2dbc255

WIP

e657a44

removed transport, replaced with guzzle default

139af24

linter

e6c922e

Fix error handling finally

93bf65c

more lint

6cadc22

debug failing test

314c40f

test fix

af0ac15

with composer.lock

5bed502

lock compatible with 8.1

0f28880

increase timeout may help?

cab8a8f

add wait for mock server to acceptance tests

36258c8

missed the type here

adadd03

seba-aln marked this pull request as ready for review March 11, 2025 10:31

seba-aln requested review from Xavrax and marcin-cebo as code owners March 11, 2025 10:31

marcin-cebo reviewed Mar 12, 2025

View reviewed changes

Sebastian Molenda added 3 commits March 17, 2025 14:53

Handle invalid response

95fa303

lint

80638ee

added example to use custom client implementation

cb8453b

marcin-cebo approved these changes Mar 18, 2025

View reviewed changes

lint

2fd3d6c

PubNub SDK 8.0.0 release.

e9c05ca

pubnub-release-bot requested review from kazydek and techwritermat as code owners March 19, 2025 13:31

techwritermat reviewed Mar 19, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

a967560

techwritermat approved these changes Mar 19, 2025

View reviewed changes

seba-aln merged commit eeee989 into master Mar 19, 2025
8 checks passed

seba-aln deleted the refactor/replace-transport-with-guzzle branch March 19, 2025 13:41

		@@ -6,9 +6,14 @@ class PNGetFileDownloadURLResult
		{
		protected string $fileUrl;

Conversation

seba-aln commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

seba-aln commented Mar 19, 2025

Uh oh!

Uh oh!

Uh oh!

pubnub-release-bot commented Mar 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seba-aln commented Feb 25, 2025 •

edited

Loading