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

centrifugo v5 new HTTP API version support #63

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

AntistressStore
Copy link
Contributor

@AntistressStore AntistressStore commented Jun 10, 2024

Added support for the new centrifugo v5 HTTP API. New headers generator for centrifugo v5.
https://centrifugal.dev/blog/2023/06/29/centrifugo-v5-released#new-http-api-format
It is possible to use "old versions" centrifugo.
If we set $client->setCompatibility(true). Then the old version of generating urls and headers will be used.
By default we use v5 version ($compatibility == false).
New test for url generate added.
All test passed successful.

@AntistressStore
Copy link
Contributor Author

I think this is a php version conflict.
I'm using the newest one, and all test successfull. On lower versions we need to write in the test
$reflectedMethod->setAccessible(true)
I'll do it and send you an update

@FZambia
Copy link
Member

FZambia commented Jun 11, 2024

@AntistressStore hello, thanks! What do you think about also dropping $client->setCompatibility(true) option - I don't think we need to maintain compatibility with older API format, will just release phpcent v6.

And another question if you don't mind, need an opinion from PHP developer, we now have:

public function setUseAssoc($useAssoc)
    {
        $this->useAssoc = $useAssoc;
        return $this;
    }

Is it a good idea to use assoc by default, or is it better to keep it off by default? I remember it caused some confusion previously, so maybe it's a good time to change if we release phpcent v6

@AntistressStore
Copy link
Contributor Author

@AntistressStore hello, thanks! What do you think about also dropping $client->setCompatibility(true) option - I don't think we need to maintain compatibility with older API format, will just release phpcent v6.

And another question if you don't mind, need an opinion from PHP developer, we now have:

public function setUseAssoc($useAssoc)
    {
        $this->useAssoc = $useAssoc;
        return $this;
    }

Is it a good idea to use assoc by default, or is it better to keep it off by default? I remember it caused some confusion previously, so maybe it's a good time to change if we release phpcent v6

Decoding to PHP arrays is not always symmetric or may not always return what you expect. On the other hand, decoding to stdClass objects (by default) is always symmetric. Arrays can be somewhat easier to work with/transform than objects.

You may lose some of the exact information from the original centrifuge response if you convert the response to the default associative array yourself. In general, this is not critical.

$a = json_decode('{"0":1,"1":2,"2":3,"3":4}',true)
= [
    1,
    2,
    3,
    4,
  ]

array_is_list($a)  
= true

$a = json_decode('{"0":1,"1":2,"2":3,"3":4}',false)
= {#6660
    +"0": 1,
    +"1": 2,
    +"2": 3,
    +"3": 4,
  }

If it is more convenient for the user to receive a ready-made array, he can independently configure the conversion, as it is done now. Since php 7.2.0, the associative parameter in json_decode is now nullable and null by default.
I.e. here, the question is between accuracy and user friendliness.
I would also prefer to get an associative array type result, instead of object(stdClass).
At the same time, convenience may turn out to be a subjective choice of some group, this is not PHP Standards Recommendations (PSR).

@AntistressStore
Copy link
Contributor Author

AntistressStore commented Jun 12, 2024

Regarding compatibility, I understand you. Then it's just that those who use versions of the centrifuge younger than version 5 will install only versions of php cent <=5. I will edit conditional compatibility checks to the only option with the new version api

@FZambia FZambia requested review from FZambia June 13, 2024 14:43
Copy link
Member

@FZambia FZambia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks @AntistressStore !

Regarding assoc question, since there are pros and cons I suppose we can change its behavior at some point in the future, but not now.

I'll merge this PR and release a new version of phpcent (v6.0.0) very soon, hope to make it during next week.

@FZambia FZambia merged commit 2fe5924 into centrifugal:master Jun 20, 2024
2 checks passed
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