-
Notifications
You must be signed in to change notification settings - Fork 2
chore: add docblocks for autocompletion #280
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
Conversation
|
@bytestream it looks like the package you use for the models doesn't support casting to other models, which is what we need to make this work. The below works for us but I'm not sure how open you are adding this to the abstract model. protected function castAttribute($key, $value)
{
if (is_null($value)) {
return $value;
}
$castType = $this->casts[$key] ?? null;
if ($castType && str_starts_with($castType, 'array:')) {
$className = substr($castType, 6);
if (class_exists($className) && is_array($value)) {
return array_map(function ($item) use ($className) {
return is_array($item) ? new $className($item) : $item;
}, $value);
}
}
if ($castType && class_exists($castType) && is_array($value)) {
return new $castType($value);
}
return parent::castAttribute($key, $value);
} |
|
I have added the casting part to the PR for now so we can use our fork in the meantime, as it's currently blocking our implementation. It has been tested with tickets, messages and operators and we haven't found any issues with it so far. |
|
Hi @ju5t, Thanks for the PR. I've updated it to at least get it passing with our test suite, however there is one clear issue I can see. There are many attributes which are nullable, such as User model I've run out of time today, if you would be interested in looking at that, that would be helpful. The other issue is that this somewhat reverses the work we did to ensure that the client is compatible with many versions of SupportPal. However we would be happy to consider that the docblock is up to date with the latest version of SupportPal, and may be wrong if you are using an older version, as we can see the benefits it gives in terms of development. |
I have most of it done. A few models haven't been done as I'm not sure what table they point to. I made a small script that reads the database structure and returns if The following tables I still need to check: - src/Model/Core/WhitelistedIp.php
- src/Model/SelfService/Attachment.php
- src/Model/SelfService/Category.php
- src/Model/SelfService/Tag.php
- src/Model/SelfService/Type.php
- src/Model/Shared/CustomField.php
- src/Model/Shared/Option.php
- src/Model/Ticket/Attachment.php
- src/Model/Ticket/Channel.php
- src/Model/Ticket/Extra.php
- src/Model/Ticket/Message.php
- src/Model/Ticket/Priority.php
- src/Model/Ticket/Status.php
- src/Model/Ticket/Tag.php
- src/Model/User/Domain.php
- src/Model/User/Group.php
- src/Model/User/Organisation.php
- src/Model/User/UserCustomField.php
The only things that would be backwards incompatible with 7.4 would be the |
|
@jshah4517 I don't know what this one is: But I think I got all the other ones covered now. |
|
@jshah4517 is there anything I can do to get this merged and released? |
|
@ju5t It looks good, thanks. Will release a new version shortly. Version 2 of the API client supports SupportPal 5.0+ which is PHP 8.1 minimum, so that's not an issue. |
Since v2.0.0 switched to direct property access you couldn't use autocompletion anymore. This PR adds docblocks to all of the models. So now we can do
$ticket->department->namefor example, and PHPStorm knows what you're doing.Important
As PHPStan is on level 8 it doesn't allow iterables without a value type. I don't know what each property holds so I don't want add
mixed. There are 32 errors at the moment.I did use some AI to infer types and add some docblocks to save time. I've done a quick review and do believe everything matches. I think its important to be transparent on AI use for PR's.