-
Notifications
You must be signed in to change notification settings - Fork 91
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
Support for Members and Membership type, status and include objects #114
Conversation
* @param string $ch | ||
* @return $this | ||
*/ | ||
public function channel($ch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
public function channel(string $ch): self
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed for uniformity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would check all files in this PR for this unification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully I didn't miss anything
protected bool $endpointAuthRequired = true; | ||
protected string $endpointHttpMethod = PNHttpMethod::PATCH; | ||
protected int $endpointOperationType = PNOperationType::PNRemoveMembersOperation; | ||
protected string $endpointName = "ManageMembers"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. it's to lower the amount of copypaste between files. Down at the bottom you see that I removed a lot of boilerplate methods and in the meantime in Endpoint
class I expose the values through proper methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
$this->endpointConnectTimeout = $this->pubnub->getConfiguration()->getNonSubscribeRequestTimeout(); | ||
$this->endpointRequestTimeout = $this->pubnub->getConfiguration()->getConnectTimeout(); | ||
} | ||
|
||
/** | ||
* @param string $ch | ||
* @return $this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be:
public function channel(string $ch): self
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would check all files in this PR for this unification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -35,6 +59,8 @@ public function channel($ch) | |||
|
|||
/** | |||
* @param array $uuids | |||
* @deprecated Use members() method | |||
* | |||
* @return $this | |||
*/ | |||
public function uuids($uuids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be:
public function uuids(array $uuids): self
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a bit more complex than just say it's an array (phpStan watches for doc block describing what type of input it should be and since it is deprecated I omited it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
* @param PNChannelMember[] $members | ||
* @return $this | ||
*/ | ||
public function members(array $members) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be:
public function members(array $members): self
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
$this->status = $status; | ||
} | ||
|
||
public function setchannel(string $channel): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setChannel ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
return $this->custom; | ||
} | ||
|
||
public function getType(): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
public function getType(): ?string
public function getStatus(): ?string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} | ||
|
||
/** | ||
* @return string[] | \StdClass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be:
- @return mixed
- ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well not exactly because mixed
says it can be a primitive type yet here we may have an array or an StdClass
object so it's a bit more precise
/** | ||
* @return string[] | ||
*/ | ||
public function toArray() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
public function toArray(): array
?
*/ | ||
function __construct($id, $name, $description, $custom = null, $updated = null, $eTag = null) | ||
{ | ||
public function __construct( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
public function __construct(
string $id,
string $name,
string $description,
mixed $custom = null,
?string $updated = null,
?string $eTag = null,
?string $status = null,
?string $type = null
)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. this object is made from a response so it would require more attention to do a proper strict typing, so I would leave the docblock type hints only
6725275
to
d4fc4a1
Compare
@pubnub-release-bot release |
🚀 Release successfully completed 🚀 |
feat: Support for Members and Membership type, status and include objects
Extended functionality of Channel Members and User Membership. Now it's possible to use fine-grade includes and set member/membership status and type.