[WIP] [BC] General project maintenance #236
Conversation
Standardization of PSR-2 as well as some basic, slightly opinionated cleanup for readability - more to come in later commits
Standardization and cleanup
dependency management
Implementing PSR-4 autoloading for development
PSR-4 development
Adding tests to encompass \Minishlink\WebPush\Notification as well as resolving undefined index notices in Notification::getOptions()
Notification tests
Simplifying and hiding away implementation details of MessageSentReport as well as fixing two broken test cases which should have never passed. SEE: MessageSentReportTest::testJsonSerialize & generateReportsWithJson
Simplifying MessageSentReport
fixing a few static analysis issues i've let slip through the cracks
Clear separation of unit and integration tests will allow for an easier on-boarding experience as well as CI interactions for developers as they no longer have to inspect each testcase individually to understand why so many are being skipped, failing, or otherwise
Separating test suites
|
Cool, great that you're motivated! Do you have a tool like prettier setup? Because if you're going to edit the whole project syntax, please setup something like this first. Otherwise someone might end up editing the whole project with another syntax afterwards. Note that php-cs-fixer is already used. |
|
Please don't skip the integration tests. It's our guarantee that modifications are valid. If they're broken, find the reason why and fix that, but please don't skip them |
|
Can you do more atomic Pull requests ? For example I like your PR to separate unit and integration tests, but there are also edits that change the behaviour of the tests and things that are not related to this. |
Sure thing, jumping into #228 with the author it looks to be that part of the issue at least is the upstream web-push-testing-service is failing. I also had issues running it on more recent versions of node so decided to follow up on it in a later commit. Added that one to the to-do list.
Thanks! I've been looking at options for this and definitely plan on committing something within this PR.
Yes, definitely can! |
|
I'll explore moving the web-push-testing-service into web-push-libs to share wider maintanence of it. |
|
@gauntface is the WPTS able to work with multiple versions of ChromeDriver? IIRC, it's using chromedriver 2.46 which supports Chrome 71-73, but now we have:
Sorry for hijacking this pull request -- we can split this off in a new issue if this needs further discussion. |
…munications, adjusted tests accordingly
Implement HTTP client
|
Instead of a concrete http client, you should rely on the PSR18 and its interfaces package. It is way more efficient |
Looking to move forward later w/ an implementation of PSR17,18 in conjunction w/ httplug's async http client interface if this PR is well received. This will give a nice happy path for refactoring from though so I don't have to vest too much time to potentially not have it accepted 😅 |
|
Moving forward I'm noticing a few inconsistencies in logic and test scenarios regarding VAPID + GCM which has me curious: considering the scope of this already warranting a major version change, what is the general consensus on dropping support for GCM? /cc @Minishlink @Spomky (& anyone else using/contributing to this project) |
Introducing `Minishlink\WebPush\Options` including removing `batchSize` as it does not apply to the notification options.
Decoupling the http client from any specific third party library Updating MessageSentReport due to differences between guzzle promises and the implementation provided (based on promisesaplus specification)
* Implement Payload to keep the string payload it's encryption components together * Implement Queue to handle queueing of notifications and promises * Implement Cache to handle caching vapid headers * Implement SubscriptionInterface and AuthorizationInterface to allow for easier integration of the library into consumer code
|
Outside of the failing integration test, which is dependent on the supporting NPM package, I feel confident and am happy with the changes so far. If anyone is willing to provide a review on the PR as a whole, or would like to see anything immediately change, I'm more than willing to work with any requests to make this PR happen. There are some more things I'd still like to work with, but they're all internal and would not affect the (newly proposed) public facing API. For example, the usage of classes internal to third-party libraries within the Encryption logic. |
|
That’s a lot of changes! The code look cleaner and should be easier to maintain. |
Thanks! I really appreciate the feedback and help working through this PR as a whole! 😃 |
Add PHP 7.4snapshot support to Travis config
…ies which will need to be addressed first
Pulling in the web-push-testing-service locally, I was able to bump the dependencies and get *most* of the service's test cases to run - enough so to be able to run and refactor the PushService integration test. Also found a few bugs along the way
|
Integration tests are passing 🎉 ... locally... The web-push-testing-service's test suite seems to have a few issues - I'm thinking relative to the fact that GCM is deprecated, but didn't actually dig into it. However, bumping the geckodriver and chromedriver, I was able to get the PushServiceTest to run, pass, and cleaned up. |
|
Now waiting on GoogleChromeLabs/web-push-testing-service#24 (or similar - if anyone is interested) and we'll be able to run those integration tests as part of CI. |
|
Still just waiting on @gauntface to check out the above PR (GoogleChromeLabs/web-push-testing-service#24) before the integration tests can be run as part of CI. Also waiting to receive any input from @Minishlink. Hoping to get the traction necessary to push this forward as quickly as possible 🙂 . |
|
👋 |
|
You can force using your branch for web-push-testing-service in the Travis configuration file EDIT: apparently the fix doesn't work https://travis-ci.org/web-push-libs/web-push-php/jobs/559862577 |
It shouldn't be running against beta channels, those were removed due to required driver changes. Not sure how those are running though, let me take a look. Edit: Ah those also look to be running against GCM which has been removed entirely now afaik. Ignore the beta comment, that was the "nightly" [and "old"?] channel I was thinking of. 2: It does look like Firefox beta channel is failing though (unable to get a subscription), going to dig into this. 3: Interesting... Firefox beta seems to pass in the web-push-testing-service test suite. |
|
At a loss as to why only Firefox beta seems to fail to receive a subscription. However, it does seem to be something relative to the web-push-testing-service/geckodriver dependency - drawing that conclusion based purely off of where the failure is happening. There are some relative issues around the selenium/browser testing ecosystem but that doesn't explain why the same test passes within the web-push-testing-service suite. What is even stranger is this failure was not happening at the time of the PR to googlechromelabs/web-push-testing-service. Any thoughts or input would be greatly appreciated. |
|
disappointing this was never merged in |
|
Is there a separate project available with these changes? |
|
Unfortunately, no, these changes no longer live anywhere - at least publicly that I know of. The process to get these changes merged in far exceeded my expectations and those put on me by my employer at the time which had allowed me to contribute to this library as part of a larger project. My contributions to the googlechromelabs/web-push-testing-service also seem to still be pending so it’s probably best I didn’t hold my breath. |
I've reached out to @Minishlink hoping to be able to collaborate on a roadmap for the project and shoot some ideas that he may be willing to accept into the project but haven't heard back yet. Hopefully, this will come as a sign of good faith and jump-start that process.
Sourcing projects similar in function to fit a need in another project, I stumbled across this library as it seems to be the defacto standard for web push with PHP. I also noticed it has some great proposed contributions from some awesome contributors and decided that rather than building my own library, why not contribute back to the community.
For anyone interested, this pull request will be a WIP and any feedback on particular changes being added in or removed are entirely welcome.
Changes in this pull request
\Minishlink\WebPush\Notification\Minishlink\WebPush\MessageSentReport[BC]Additional pull requests to be considered for version 6
Questions
travis.ymlis partially configured to run the entirety of our test suite, integration tests included - do we want to look into running our integration tests in CI, or clean up the otherwise unneeded configuration?