search

web-push-libs / web-push-php

Web Push library for PHP
MIT License
1.68k stars 295 forks source link

Make use of Guzzle Pool to improve efficiency #401

Closed Gugu7264 closed 1 month ago

Gugu7264 commented 1 month ago

Fixes #367, fixes #195.

Since making use of the Pool refrains from returning anything to the user, a callback argument is added in the new flushPooled function, which will get called for each response received.

It was mentioned in #367 to transform the prepare method into a Generator, but that's not possible if we want to be able to return the Request alongside the Response.

Minishlink commented 1 month ago

Thanks @Gugu7264 for the PR and your help on the other issues.

  1. Is there any advantage to use flush instead of flushPooled? Was flushPooled added to prevent a breaking change? If so, next version will have several breaking changes so we could group them together.
  2. Apparently some tests are failing
  3. Can you add a test for when the user chooses flushPooled instead of flush please? (depending on the answer of 1 ;) )
Gugu7264 commented 1 month ago

It's indeed added as a separate function because of the breaking change behavior of using a callback instead if returning/yielding something. Apart from that, I do not think there's any advantage. Will fix tests and add tests

Gugu7264 commented 1 month ago
  1. Apparently some tests are failing
  2. Can you add a test for when the user chooses flushPooled instead of flush please? (depending on the answer of 1 ;) )

@Minishlink Here are 2 commits that should fix 2. and 3. I haven't done community PHP development before and wasn't aware of the tests set up in composer, hence the failing (I was missing a comma...). I've now run the tests, apart from test:unit (but I ran test:unit_offline) so everything should be good now :)

Gugu7264 commented 1 month ago
  1. Is there any advantage to use flush instead of flushPooled? Was flushPooled added to prevent a breaking change? If so, next version will have several breaking changes so we could group them together.

As for this, I added this as separate since I don't know when will next version come, but I can change it for the next breaking version? (maybe there should be a separate branch for the breaking version?)

Gugu7264 commented 1 month ago

Here's a fixed version @Minishlink