Skip to content
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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

Garethp
Copy link
Owner

@Garethp Garethp commented Aug 20, 2024

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.

  • Fix the Code Generation so that we can generate new types again
  • Replace is* magic methods with Concrete implementations
  • Replace get* magic methods with Concrete implementations
  • Replace add* methods with Concrete implementations
  • Replace set* methods with Concrete implementations
  • Automatically cast types in the generated methods if they are detected in the typemap
  • Update all the PhpUnit metadata to use attributes instead
  • Inline the magic casting so that we can remove MagicMethodsTrait entirely
  • Document the code generation
  • Update the code generator so that we can define return types
  • Update code style to psr-12

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

@Garethp Garethp changed the title Remove all uses of the Magic Methods by regenerating our classes to have concrete implementations instead [Draft] Remove all uses of the Magic Methods by regenerating our classes to have concrete implementations instead Aug 20, 2024
@marclaporte
Copy link

@Garethp This is fantastic!

Over the next few weeks, we will integrate php-ews into Cypht webmail:
cypht-org/cypht#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
Copy link
Owner Author

Garethp commented Aug 23, 2024

@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
Copy link
Owner Author

Garethp commented Aug 23, 2024

@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
Copy link

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants