vineyardkoeln / churchtools-api

Access the API for ChurchTools at https://api.churchtools.de/ in an object-oriented way that integrates nicely with modern PHP projects via Composer.
Apache License 2.0
14 stars 8 forks source link

Use object model for master data and other API calls #4

Closed a-schild closed 5 years ago

a-schild commented 5 years ago

Hello,

I have written some classes to be used with the API, which encapsulate the calendar, events and parts of the bookings stuff of ct.

Before I make a pull request, how should I integrate it with the existing API?

  1. Change api calls to return the objects (Would not be backward compatible)
  2. Add separate calls which return object and leave the existing calls as they are?

Currently it would be the getCalendarEvents which I have implemented with an object model, and also the calendar part of the masterdata.

YetiCGN commented 5 years ago

That's great! It says in the README: "TODO: In one of the upcoming alpha versions, methods will return model objects and not plain arrays."

So I was initially planning to do this, but I haven't yet got the time to do it. Feel free to submit your changes with backwards incompatibility, since this project is still in alpha state anyway. I don't see the use case of separate calls, that would probably just be confusing and working with domain objects is much nicer.

People who need the array data can peg their version constraint to ~0.3.0 easily.

I would just ask you that your code be PSR-2 compliant. 🙂

a-schild commented 5 years ago

@YetiCGN Currently you define php 7.0.9+ as required PHP version. Can I bump this to 7.1+, so we can define nullable arguments + void return types? One problem with php 7.1+ might be, that all debian 9x version only ship with php 7.0x and you have to add other package sources for php 7.1, 7.2 7.3 support.

PS: PSR-2 on focus ;)

YetiCGN commented 5 years ago

Yes, you can do this. Actually I came across this yesterday as well and thought "if we ever want nullable types, we need to raise this to 7.1 at least". 😆

7.0 is no longer supported, so it's safe to drop support for it if that means we can have better code because of correct strict typing. Users of Debian Stretch should switch to Ubuntu or install the ~PPA~ DPA. 😉

a-schild commented 5 years ago

See #5 for a pull request. Most things work, but not yet widely tested

YetiCGN commented 5 years ago

Merged, thanks a lot!