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

Various upgrades and modernisation #3

Closed
wants to merge 3 commits into from

Conversation

NoelLH
Copy link

@NoelLH NoelLH commented Jul 27, 2021

  • Update to modern libraries, including avoiding those with published security issues
  • Add roave/security-advisories to check on update for future upstream library security issues
  • Remove support for PHP versions no longer in active maintenance; add support for modern versions
  • Replace non-binary safe fopen() calls with flexible PSR-3 log adapter support
  • Make extension requirements explicit in composer.json
  • Expand use of types and strict comparisons
  • Other minor tidying, PSR-2 fixes & removal of some dead code

* Update to modern libraries, including avoiding those with published security issues
* Add `roave/security-advisories` to check on update for future upstream library security issues
* Remove support for PHP versions no longer in active maintenance; add support for modern versions
* Replace non-binary safe `fopen()` calls with flexible PSR-3 log adapter support
* Make extension requirements explicit in `composer.json`
* Expand use of types and strict comparisons
* Other minor tidying, PSR-2 fixes & removal of some dead code
@NoelLH
Copy link
Author

NoelLH commented Jul 27, 2021

@JustinBusschau please take this with a small pinch of salt for now, as I've not yet tried it out downstream with e.g. hmrc-gift-aid. I'm hoping it is close to something that could be considered for a new v1 though – necessarily there are breaking changes because of Guzzle changing their namespace, PSR-3 support etc.

I have one potentially key question so far I'd like to clarify: What's an example of $fullRequestString? Is appending it directly as I've done on https://github.com/JustinBusschau/php-govtalk/pull/3/files#diff-a19fa425830a9ea8a2075e0675738988bdc1e05740ec4b98f838e4dceccd7c48R1152 correct / plausible? Based on the base URL example in the test code this would suggest it should start with a / when non-null. Either way it would be good to document what's expected / supported I think.

It would be great to hear any thoughts you have generally, and whether you think this sits best in your fork or is something it would make sense for us to publish separately under thebiggive's namespace.

NoelLH added a commit to thebiggive/hmrc-gift-aid that referenced this pull request Jul 27, 2021
See JustinBusschau/php-govtalk#3 for a guide to
what some of the remaining changes will need to look like. To maintain
Guzzle library cross-compatability etc., another requirement for this
update will be for the upstream `php-govtalk` to be swapped over to its
v1.x (if updated in situ) or to the `thebiggive` fork.
@JustinBusschau
Copy link
Owner

@NoelLH at a quick glance the changes look good. To be frank, I don't have the time to maintain this repo. I'm very happy it's been useful to you - perhaps you should take a fork and move it to thebiggive namespace?

@NoelLH
Copy link
Author

NoelLH commented Sep 7, 2021

Thanks for confirming @JustinBusschau, and for all your work on this in the past – much appreciated.

I'll close this and am proceeding in our namespace at https://github.com/thebiggive/php-govtalk as you suggest.

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