Garethp / php-ews

PHP Exchange Web Services
BSD 3-Clause "New" or "Revised" License
112 stars 45 forks source link

[Draft] Remove all uses of the Magic Methods by regenerating our classes to have concrete implementations instead #265

Open Garethp opened 2 months ago

Garethp commented 2 months ago

In order to upgrade to PHP 9, we need to remove dynamic property declaration. The main problem is the usages of that we have in the __set method of MagicMethodsTrait. If we found another of doing that we could probably scrape by, but to be safe we should convert over to removing magic methods entirely and creating concrete implementations when we generate the classes.

There are a number helper functions I've added to a small number of classes on request that I'd like to auto generate as well, such as having methods that state they're returning arrays actually do so. However doing that, although it makes for a consistent interface, is a rather large breaking change. We'll try for that for a 2.0 release. This MR will attempt to be backwards compatible while removing usages of deprecated features.

I'd also like to generate signatures that come with return types, but that's going to require some more work around changing how classes are generated. The current packages that we're extending from won't allow that.

Edit: Updating the generator to add in return types is easy enough on its own however it has many knock-on effects since everything gets type-checked the whole way through. That's not unsolvable, we just need to adjust the types of properties and massage the shape more coming in and going out. The problem is that my integration tests only return the responses and not the requests so that we don't accidentally capture sensitive information, meaning that we don't have any assertions on the messages going out. This hasn't mattered since none of the updates so far have changed the shapes of objects, but to our objects to have the same shape that our typings want we will need to do that.

This is all to say that this falls under the next major version release, not this one. Not only will it be a change of interface for consumers but it's also going to require some manual testing, which means that I'll have to sign up for some new Exchange accounts and get this connected to Office 365. It's a large enough piece of work that I'd rather not fold it into this already large piece of work

marclaporte commented 2 months ago

@Garethp This is fantastic!

Over the next few weeks, we will integrate php-ews into Cypht webmail: https://github.com/cypht-org/cypht/issues/247

So the Cypht community can help with testing. And we can help with the simpler coding tasks (we are new to the code base so we can't do complex things but we can do simpler repetitive things). We'll be using PHP 8.1+.

Thanks!

Garethp commented 2 months ago

@marclaporte, I'm glad to hear that, thanks! Manual testing will be great once I'm close to being done would be really helpful.

If you think it falls under simpler coding tasks, contributing some tests would be rather high impact. The tests in this project are almost all integration tests. In the root of the project there are three phpunit config files, (phpunit.xml, phpunit.record.xml and phpunit.live.xml) which just change whether the integration tests run off of pre-recorded HTTP Responses, Live HTTP Calls (with connection settings defined in tests/src/BaseTestCase.php or Live HTTP Calls while recording the responses to files in the Resources/recordings folder.

Being integration tests, they're mostly about sending requests in through the surface API and asserting outputs however they will ensure all of the translations of data-shape underneath. If you find yourself with the time and comfort-level to contribute some tests and recordings that fit your use-case then that'll help prove the stability of the PHP9 upgrades that I make.

Garethp commented 2 months ago

@marclaporte, due to my limited knowledge and use-case of Exchange at the time that I made this library (and I can't say it's expanded since then) you may find the surface area of the developer-friendly simple API Classes to be smaller than you want. I'm more than happy to either take in new contributions or requests to expand the developer-friendly API Classes so that you don't have to rely too heavily on EWS's lower-level API Calls.

marclaporte commented 2 months ago

If you find yourself with the time and comfort-level to contribute some tests and recordings

Yes, this is within our reach and I have added to our todo list.

marclaporte commented 2 months ago

I'm more than happy to either take in new contributions or requests to expand the developer-friendly API Classes

Ok, great! We'll start with the basic use cases, and expand progressively from there.

marclaporte commented 1 week ago

Over the next few weeks, we will integrate php-ews into Cypht webmail:

The work has started: https://github.com/cypht-org/cypht/pull/1278