norkunas / onesignal-php-api

OneSignal API for PHP
MIT License
234 stars 83 forks source link

Implement View Outcomes endpoint #150

Closed andriichuk closed 3 years ago

andriichuk commented 3 years ago
  1. Implemented View Outcomes API method.
  2. Introduced Enums for convenient group work with device types and outcome filters to typo mistakes. What do you think about this?
  3. Did some manual encoding for outcome_names filter key to resolve issue with numeric keys in URL.
  4. Improved readme from previous PR.

Closes: https://github.com/norkunas/onesignal-php-api/issues/140

norkunas commented 3 years ago

Overall looks good, but I'm not sure it's worth to add a dependency for enums. I'd wait for 8.1 release to use native enums :)

norkunas commented 3 years ago
  1. Did some manual encoding for outcome_names filter key to resolve issue with numeric keys in URL.

So http_build_query wrongly builds the query?

andriichuk commented 3 years ago
  1. Did some manual encoding for outcome_names filter key to resolve issue with numeric keys in URL.

So http_build_query wrongly builds the query?

Yes, it encodes like outcome_names[0]=click, but we need outcome_names[]=click without array index

norkunas commented 3 years ago

I think preg_replace would be enough after http_query_build then

andriichuk commented 3 years ago

Overall looks good, but I'm not sure it's worth to add a dependency for enums. I'd wait for 8.1 release to use native enums :)

After 8.1 release we can remove this dependency 😄. With enums it easier to manage groups of constants. If you don't like it I can remove this dependency

norkunas commented 3 years ago

Overall looks good, but I'm not sure it's worth to add a dependency for enums. I'd wait for 8.1 release to use native enums :)

After 8.1 release we can remove this dependency . With enums it easier to manage groups of constants. If you don't like it I can remove this dependency

Constants are just constants so adding a dependency could introduce conflicts for consumers :) And then would need to deprecate the old ones and people would need to learn again so I'd prefer leaving as is. :)

andriichuk commented 3 years ago

Didn't think about it, yes it make sense, removed enums.

andriichuk commented 3 years ago

Squashed commits

norkunas commented 3 years ago

Thank you @andriichuk :)