Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bad phpstan documentation for WebPushNotificationShape #893

Closed
adpeyre opened this issue May 20, 2024 · 8 comments · Fixed by #895
Closed

Bad phpstan documentation for WebPushNotificationShape #893

adpeyre opened this issue May 20, 2024 · 8 comments · Fixed by #895
Assignees

Comments

@adpeyre
Copy link

adpeyre commented May 20, 2024

Describe the bug

Hello,

Phpstan documentation is :

/**
 * @see https://tools.ietf.org/html/rfc8030#section-5.3 Web Push Message Urgency
 *
 * @phpstan-type WebPushHeadersShape array{
 *     TTL?: positive-int|numeric-string|null,
 *     Urgency?: self::URGENCY_*
 * }
 *
 * @see https://firebase.google.com/docs/reference/fcm/rest/v1/projects.messages#webpushfcmoptions WebPush FCM Options
 *
 * @phpstan-type WebPushFcmOptionsShape array{
 *     link?: non-empty-string,
 *     analytics_label?: non-empty-string
 * }
 *
 * @see https://developer.mozilla.org/en-US/docs/Web/API/Notification/Notification#syntax WebPush Notification Syntax
 *
 * @phpstan-type WebPushNotificationShape array{
 *     title: non-empty-string,
 *     options?: array{
 *         dir?: 'ltr'|'rtl'|'auto',
 *         lang?: string,
 *         badge?: non-empty-string,
 *         body?: non-empty-string,
 *         tag?: non-empty-string,
 *         icon?: non-empty-string,
 *         image?: non-empty-string,
 *         data?: mixed,
 *         vibrate?: list<positive-int>,
 *         renotify?: bool,
 *         requireInteraction?: bool,
 *         actions?: list<array{
 *             action: non-empty-string,
 *             title: non-empty-string,
 *             icon: non-empty-string
 *         }>,
 *         silent?: bool
 *     }
 * }
 *
 * @see https://firebase.google.com/docs/reference/fcm/rest/v1/projects.messages#webpushconfig WebPush Config Syntax
 *
 * @phpstan-type WebPushConfigShape array{
 *     headers?: WebPushHeadersShape,
 *     data?: array<non-empty-string, non-empty-string>,
 *     notification?: WebPushNotificationShape,
 *     fcm_options?: WebPushFcmOptionsShape
 * }
 */

With this shape, it does not work.
I have to use :

WebPushConfig::fromArray([
                'notification' => [
                    'title' => 'title',
                    'body' => 'body',
                    'icon' =>  'icon',

                ],

It's a problem because if I don't use the documentated shape, PHPStan is not happy.

Parameter #1 $config of static method Kreait\Firebase\Messaging\WebPushConfig::fromArray() expects
         array{headers?: array{TTL?: int<1, max>|numeric-string|null, Urgency?: 'high'|'low'|'normal'|'very-low'},
         data?: array<non-empty-string, non-empty-string>, notification?: array{title: non-empty-string, options?:
         array{dir?: 'auto'|'ltr'|'rtl', lang?: string, badge?: non-empty-string, body?: non-empty-string, tag?:
         non-empty-string, icon?: non-empty-string, image?: non-empty-string, data?: mixed, ...}}, fcm_options?:
         array{link?: non-empty-string, analytics_label?: non-empty-string}}, array{notification: array{title: string,
         body: string, icon: string|null}, fcm_options: array{link: string|null}} given.

Ty

Installed packages

7.6.0

PHP version and extensions

php 8.1

Steps to reproduce the issue.

# Insert the commands issued in the terminal if they are needed
# to reproduce the issue. Otherwise, delete this code block.
// Insert the PHP code to reproduce the issue. Please ensure that it is code that
// can be copy pasted to reproduce it.

Error message/Stack trace

...

Additional information

No response

@jeromegamez
Copy link
Member

Thank you for the report, I'll look into it. Could you please update your message and replace "Last Version" with the actual version you have installed?

@jeromegamez
Copy link
Member

You said "With this shape, it doesn't work" - could you give me an example of a shape that doesn't work?

@adpeyre
Copy link
Author

adpeyre commented May 20, 2024

Ty. Sure !

Phpstan excepts this shape :

$message = CloudMessage::new()
            ->withWebPushConfig(WebPushConfig::fromArray([
                'notification' => [
                    'title' => 'title',
                    'options' => [
                        'body' => 'body',
                    ]
                ],
            ]));

But it's wrong.
I have to write that. Your documentation is correct at https://firebase-php.readthedocs.io/en/7.11.0/cloud-messaging.html#webpush :

$message = CloudMessage::new()
            ->withWebPushConfig(WebPushConfig::fromArray([
                'notification' => [
                    'title' => 'title',
                    'body' => 'body',
                ],
            ]));

For information, I've tested these parameters which work on chrome mobile (android) and dekstop (windows).

        $message = CloudMessage::new()
            ->withNotification(['title' => 'Notification title', 'body' => 'Notification body'])
            ->withWebPushConfig(WebPushConfig::fromArray([
                'notification' => [
                    'title' => 'title',
                    'body' => 'body',
                    'icon' =>  '/uploads/files/662f9d3ca6a9c166680163.png',
                    'badge' =>  '/uploads/files/662f9d3ca6a9c166680163.png',
                    'image' =>  '/uploads/files/662f9d3ca6a9c166680163.png',
                    'actions' => [
                        ['action' => 'testaction', 'title' => 'Title action', 'icon' => '/uploads/files/662f9d3ca6a9c166680163.png']
                    ]
                ],
            ]));

@jeromegamez
Copy link
Member

Could you please confirm if #895 works as expected, and comment on the PR if yes or no? Thanks!

@adpeyre
Copy link
Author

adpeyre commented May 22, 2024

It's better but not perfect in my opinion. :) I don't know which is the good solution....

  1. The "non-empty-string" directive forces me to set a string directly. I can't pass by an object property with type string (maybe a phpstan bug ?)

Does not work :

$message = CloudMessage::new()
            ->withWebPushConfig(WebPushConfig::fromArray([
                'notification' => [
                    'title' => $pushNotificationMessage->title,

Works :

$message = CloudMessage::new()
            ->withWebPushConfig(WebPushConfig::fromArray([
                'notification' => [
                    'title' => 'title',

2) I liked doing the null value filter (jsonSerialize) because if don't have data for a specific parameter (null), the parameter was removed. With this documentation, I can't do that anymore. But I can apply the null filter myself.

I think we can validate this modification. The old one was wrong.

@jeromegamez
Copy link
Member

If the object providing the $title property returns a non-empty string, it should work, so I don't think it's a PHPStan bug (it almost never is 😅). I changed it now to allow it to be an empty string as well - making it optional, or allowing null is not supported because the Firebase API will reject those messages.

Let me know if this works better for you.

@adpeyre
Copy link
Author

adpeyre commented May 23, 2024

With this "non-empty-string" directive, I have to change everywhere the type of my properties. Overthise, I got this phpstan error :

Property ...$body (non-empty-string) does not accept string.

But I don't know what is the best approach...

@jeromegamez
Copy link
Member

Could we please continue the discussion on the PR, where the work is done? I already removed the non-empty-string constraint.

https://github.com/kreait/firebase-php/pull/895/files#diff-e65edab59bb22bcceb7c29269b9178b3e05697a81bf87c3010fe8ee3fbab2ec8R38

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants