-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Draft] Remove all uses of the Magic Methods by regenerating our classes to have concrete implementations instead #265
base: master
Are you sure you want to change the base?
Conversation
@Garethp This is fantastic! Over the next few weeks, we will integrate php-ews into Cypht webmail: 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! |
@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, ( 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. |
@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. |
Yes, this is within our reach and I have added to our todo list. |
Ok, great! We'll start with the basic use cases, and expand progressively from there. |
…em as safe for regeneration
…ic property assignments so that we can build anonymous objects from arrays
…test on older versions of PHP
The work has started: cypht-org/cypht#1278 |
Now in testing phase: cypht-org/cypht#1278 |
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 ofMagicMethodsTrait
. 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.is*
magic methods with Concrete implementationsget*
magic methods with Concrete implementationsadd*
methods with Concrete implementationsset*
methods with Concrete implementationsUpdate the code generator so that we can define return typesThere 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