Add event type as constants to Event class

[nickdnk]

Hello again

I often find myself using the raw string values in my project. I wanted to avoid this, so I added them as constants to the library. This avoids typos in event type strings and also makes it easy to find the event type one is looking for without having to consult the online docs.

:)

[nickdnk]

Now i'm gonna try the rebase we just talked about @brandur-stripe :)
Hold on

Edit: When I run the rebase command it throws me into some vim-editor that I cannot figure out how to use :(

[brandur-stripe]

Maybe just keep this one alphabetized with the others.

[nickdnk]

It follows the exact order of the events on https://stripe.com/docs/api#event_types
But I can move it up if you still want?

[brandur-stripe]

Yeah, move it. We should probably change it in the API reference too.

[nickdnk]

I think the reason it's at the bottom is because it's the only one that has no object attached (all others "describe" something). In other words; it's not really an event you can process in any way.

[brandur-stripe]

Yeah. I just think that in this case it'd be better to keep alphabetical order because that sort of information isn't really present. It's generally expected that this sort of list of constants is arranged.

[brandur-stripe]

Nice! Thanks for the big chunk of work here.

I like the constants, but there is also a bit of a disadvantage in that it gets easier for this canonical list of events to become outdated more easily (we already have this probably elsewhere like in PHPDoc properties so maybe it's not the end of the world though).

@ob-stripe Thoughts?

[nickdnk]

It may become outdated as in "missing some events", but it can never become outdated as in "has wrong values" as this would effectively break everyone's current implementation of webhooks.

[brandur-stripe]

Edit: When I run the rebase command it throws me into some vim-editor that I cannot figure out how to use :(

https://i.imgur.com/VOe71EA.png

Try setting EDITOR temporarily to something you know how to use. For example:

export EDITOR=nano

Then try the rebase again.

It may become outdated as in "missing some events", but it can never become outdated as in "has wrong values" as this would effectively break everyone's current implementation of webhooks.

Yes, I meant that it might be missing newer events.

[nickdnk]

@brandur-stripe
I tried following your guide from the last PR, but I end up here: https://imgur.com/a/0zuML

Not sure what to do and I'm afraid to push now which may make it worse :(

[brandur-stripe]

I tried following your guide from the last PR, but I end up here: https://imgur.com/a/0zuML

This actually looks roughly like where you want to be ... as noted in that other PR, you want to change all but the top commit to f instead of pick, write out the file, then exit.

If you didn't change anything in there, you won't have seen the "rebasing ..." message afterwards and your branch should be safe. You can also just test with the standard git push origin master — Git won't let you push a rebased branch without specifying the --force flag.

[nickdnk]

I got it. They're squashed into one commit now :) Great

[brandur-stripe]

I got it. They're squashed into one commit now :) Great

Nice work! Now just follow that process every time you add a new commit :)

I'll wait for a second opinion from OB, but otherwise this is looking good. Thanks!

[nickdnk]

I figured that most devs will likely be creating these constants on their own (at least I was), and those are just as likely (if not more) to become outdated as this list is. I was debating with myself where to add my constants but then figured I might as well complete the list and attach them to the Event class for everyone to use.

And you're welcome :) Stripe <3

[ob-stripe]

I like the idea of having constants defined in the library! Just a suggestion: do you think it would make sense to wrap all the constants in their own class/namespace, to clarify that all those constants represent event types?

[nickdnk]

I think it’s logical to have them in this class, since the Event object (as JSON) has the property “type” which is one of these constants. On 17 Feb 2018, at 00.23, Olivier Bellone <[email protected]<mailto:[email protected]>> wrote: I like the idea of having constants defined in the library! Just a suggestion: do you think it would make sense to wrap all the constants in their own class/namespace, to clarify that all those constants represent event types? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#443 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AIBYsom-0XqC1p2P4Y6fZ3bNyKR7o30qks5tVg31gaJpZM4SI_y5>.

[nickdnk]

Ill fix it tomorrow. I’m at the club in the middle of a karaoke session.

Sent from my iPhone On 17 Feb 2018, at 01.17, Brandur <[email protected]<mailto:[email protected]>> wrote: @brandur-stripe commented on this pull request.

________________________________ In lib/Event.php<#443 (comment)>:

+ const SIGMA_SCHEDULED_QUERY_RUN_CREATED = 'sigma.scheduled_query_run.created';

+ const SKU_CREATED = 'sku.created'; + const SKU_DELETED = 'sku.deleted'; + const SKU_UPDATED = 'sku.updated'; + const SOURCE_CANCELED = 'source.canceled'; + const SOURCE_CHARGEABLE = 'source.chargeable'; + const SOURCE_FAILED = 'source.failed'; + const SOURCE_MANDATE_NOTIFICATION = 'source.mandate_notification'; + const SOURCE_TRANSACTION_CREATED = 'source.transaction.created'; + const TOPUP_CREATED = 'topup.created'; + const TOPUP_FAILED = 'topup.failed'; + const TOPUP_SUCCEEDED = 'topup.succeeded'; + const TRANSFER_CREATED = 'transfer.created'; + const TRANSFER_REVERSED = 'transfer.reversed'; + const TRANSFER_UPDATED = 'transfer.updated'; + const PING = 'ping'; Yeah. I just think that in this case it'd be better to keep alphabetical order because that sort of information isn't really present. It's generally expected that this sort of list of constants is arranged. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#443 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AIBYsufaUqtwPAQPQogroV7PWXlTKQx_ks5tVhp8gaJpZM4SI_y5>.

[ob-stripe]

@nickdnk Do you mind opening a separate PR for the PHPDocs stuff? It's better if each PR only covers one self-contained set of changes.

[nickdnk]

@ob-stripe I would very much like to, but how do I do that when I already have a fork on my page with the previous PR? I am not sure how to split it into two.

[ob-stripe]

You'd need to create a new branch for the second PR, then use the cherry-pick command to pick the commit into the new branch. Something like this should work:

git checkout master
git checkout -b fix-phpdoc
git cherry-pick c93ac5c

Once you've picked the commit, you can push the new branch and open another PR.

Afterwards, you can go back to the add_event_constants branch, reset to the previous commit and force-push to update the PR:

git checkout add_event_constants
git reset --hard HEAD~1
git push origin HEAD --force

Beware that this will delete the commit, so don't do this until you're sure the commit has successfully been picked in the other branch.

[nickdnk]

@ob-stripe I think I managed to do it correctly. Thanks for the assistance :)

[ob-stripe]

Awesome :) Can I bother you to rebase this PR to squash the 2 commits into one?

[nickdnk]

@ob-stripe Done :)

[nickdnk]

Hey @ob-stripe - would you mind merging this one in? Or do I need to change something? :)

[ob-stripe]

Sorry for the delay @nickdnk -- just released this in stripe-php 6.3.1. Thanks for the contribution :)

[nickdnk]

Not a problem. I'm happy to help :)