tschoffelen / php-pkpass

💳 PHP class for creating passes for Wallet on iOS.
https://includable.com
MIT License
901 stars 185 forks source link

Add support for Wallet Orders #137

Closed cjnewbs closed 5 months ago

cjnewbs commented 5 months ago

Summary

Added support for Apple Wallet orders. Thought I'd open a draft PR to get some feedback before I go any further. I have tested that this works by creating a bare minimum test order, and AirDropping it to my iPhone.

Happy to revert all the changes I made to make the PKPass class a little bit more flexible and instead override the method in FinanceOrder.

Todo:

Will resolve #136

tschoffelen commented 5 months ago

This looks amazing! It's a lot cleaner than I thought it would be to extend the class like this, great!

Definitely mergeable if we add some docs. A test would be good indeed. I think the signing cert for the current test could be reused for the orders test. The test doesn't have to generate a signature that Apple is happy with/recognises, at least if you're happy using the same type of test setup used for the PKPass test.

cjnewbs commented 5 months ago

I have added a unit test (I'm a Magento developer so used an order in the sampledata as an example to base it on). I re-read the original comment on the issue and had another think about making some more changes to add an abstract class, but on reflection I think its probably ok as-is. If Apple add another feature that requires creating a "signed bundle/package" perhaps it'll be worth taking another look then.

For the unit test I originally thought about creating a real Apple one but as you mentioned it only really needs to succeed in signing as there is no integration test validating it on a device. I have used the same data (apart from the order and merchant identifier) to create one I have validated on device so I am happy with that. Marking as ready for review.🙂

tschoffelen commented 5 months ago

Great, I'm going to get this merged now.

In the future we might want to add some docs to explain the usage of this, but hopefully for now the new test should guide people in the right direction if they stumble upon this.

tschoffelen commented 5 months ago

Released to Packagist as 2.3.0 - thanks again @cjnewbs!

cjnewbs commented 4 months ago

Thats amazing, glad you like it 🤩 ! I realised I forgot to add any docs to this earlier last week. Just sat down to write some simple ones and noticed this merged. I'll get a few put together and create a subsequent PR at some point. Hope that's ok 🙂

tschoffelen commented 4 months ago

Of course, thank you!