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

Significant Re-Organisation and Modernisation for 5.0 Release #94

Closed
wants to merge 68 commits into from

Conversation

gsteel
Copy link

@gsteel gsteel commented Feb 18, 2022

Hi There,

I use the Postmark PHP library in a lot of projects which I've recently migrated to PHP 8.1.

Because of the invalid PHP version constraint in composer.json, this has introduced a lot of runtime errors that could have been avoided by limiting installation to tested versions of PHP.

Not content with downgrading to an older version of PHP, I've updated this library significantly to suit my needs.

There are a number of BC breaks and reviewing this is going to be a fair bit of work, but I'll do my best to summarise the changes:

  • Minimum version of PHP is now 7.4
  • Removes Circle CI configuration in favour of GitHub Actions
  • Supported PHP versions 7.4, 8.0, 8.1
  • CI Tests on "8.2" or nightly
  • Adopted the Doctrine Coding Standard to keep code consistent using PHP Code Sniffer
  • Adds Psalm for static analysis and vastly increased type safety
  • Replaces all existing tests with Unit tests that do not hit the network at all, only verifying the path, method and parameters sent to the remote API (There's a lot of scope here to improve the runtime validation of parameters)
  • All code paths covered by tests
  • Upgraded PHP Unit to the most recent version
  • Drop Guzzle as a dependency and replaces with PSR-18 client dependency and PSR-17 factories. Users will need to install, or already have a PSR-18 client installed in their project in order to use the client. Common clients like Guzzle, and php-http/curl-client all implement the PSR-18 interfaces and provide PSR-17 implementations.
  • Parameter and return type hints have been added, where possible throughout and strict_types enabled
  • Deprecated AdminClient methods have been dropped
  • The primary method names and parameter names and values for the clients have been preserved - in the case of the standard client, the methods have been split out into traits containing related methods to reduce the unwieldy size of the class.
  • Exceptions have been completely re-worked. The previous concrete exception \Postmark\Models\PostmarkException has been replaced with an interface \Postmark\Exception\PostmarkException and there are a number of newly introduced types that implement this interface

Sorry this is such a large pull request, but due to how long other pull requests have been hanging around, I've got my own fork into a position where it can be released and used on >= 8.0 without runtime errors.

Moves all existing integration tests into a Legacy namespace so they can be easily grouped and excluded as getting these tests to run is never likely to happen.
- Drop PHP versions prior to 7.4
- Setup initial dev dependencies
- Declare json extension
- Declare psr/http-client
- Fix autoload configuration
- Setup scripts
- Add lockfile to git
- Implement abstract class for re-use of Mock HTTP Client assertions
- Add method to convert booleans to strings in query parameters
- Report psalm stats in CI
- Complete unit tests for Bounce related and stats related methods
- Fix removal of 'empty' body and query parameters
- Change parameter type for $includeArchivedStreams in listMessageStreams to a boolean
- Add tests for message stream related methods
Template models when given should be pretty much anything that can serialise to JSON, but object-like nonetheless.
@gsteel
Copy link
Author

gsteel commented Nov 2, 2022

@pgraham3 - Is anyone at @ActiveCampaign able to review this? PHP 7.4 goes EOL in a week or two when 8.2 comes out. Given this PR passes on 8.2 RC, it might be a useful improvement to ensure compat with 8.2

@pgraham3
Copy link
Contributor

pgraham3 commented Nov 3, 2022

@gsteel I reached out internally at AC about this PR and getting it merged. Will follow up when I hear back from the wider team on it.

@ewood-ac ewood-ac mentioned this pull request Oct 17, 2023
@gsteel
Copy link
Author

gsteel commented Oct 30, 2023

There really is no point having this patch hanging around. Shame it was such a waste of time. :(

@gsteel gsteel closed this Oct 30, 2023
@gsteel gsteel deleted the feature/modernise branch October 30, 2023 21:50
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.

None yet

2 participants