Increase PHP requirement to 8.1 #88
Conversation
|
@igorw @WyriHaximus any thoughts, perhaps? |
@ericsizemore First glace looks awesome, but will have a thorough look once I get back from a work trip next Sunday |
Sounds good. :) --Side note, just noticed 'Unverified' next to my commits and briefly forgot that I just got my signing keys setup on GitHub and set vigilant mode 😆 Concerned me for a moment. |
A slight optimization of sorts. Instead of checking for $event being null for both listeners and onceListeners, make it a single check.
Replaces deprecated 'set-output' and removes the composer-action-hash since composer isn't part of the matrix strategy.
|
One thing I noticed with the Since |
|
Howdy @WyriHaximus, Just dropping in again, as it's been a few weeks. :) |
|
Sorry for the delay, having a look tomrrow |
No worries at all! :) |
There was a problem hiding this comment.
Loving all the things you've updated and put work into. A couple of things:
- Added 13 comments for awesome things, requested changes, and some thoughts
- Since this package is the basis for @reactphp we want to make sure it stays super performant, so I'm pinging @clue and @SimonFrings for their thoughts on this PR as well
- Did you by any chance run the benchmark scripts to measure the percentual different in performance?
- Great work, thank you for putting the time in improve it like this <3
|
@WyriHaximus Thanks for the review! :) I am currently out of town so I will take a look at this when I get home tomorrow. |
I plan to look at the requested changes and reply/work towards a new commit later today. As for benchmarks, I have some data I can share. Note: I'm on Windows 11, using PHP 8.3.7 w/Xdebug; I unfortunately do not have a linux system to do these benchmarks on. I have data from the included files in PHPBench ResultsI'm using the {
"$schema":"https://raw.githubusercontent.com/phpbench/phpbench/master/phpbench.schema.json",
"runner.bootstrap": "vendor/autoload.php"
}and namespace Evenement\Tests\Benchmark;
use Evenement\EventEmitter;
class EventEmitterBench
{
/**
* @Revs(1000000)
* @Iterations(3)
*/
public function benchEmit()
{
$emitter = new EventEmitter();
$emitter->on('event', static function (int $a, int $b, int $c): void {});
$emitter->emit('event', [1, 2, 3]);
}
/**
* @Revs(1000000)
* @Iterations(3)
*/
public function benchEmitOneArgument()
{
$emitter = new EventEmitter();
$emitter->on('event', static function (int $a): void {});
$emitter->emit('event', [1]);
}
/**
* @Revs(1000000)
* @Iterations(3)
*/
public function benchEmitNoArguments()
{
$emitter = new EventEmitter();
$emitter->on('event', static function (): void {});
$emitter->emit('event');
}
/**
* @Revs(100000)
* @Iterations(3)
*/
public function benchEmitOnce()
{
$emitter = new EventEmitter();
$emitter->once('event', static function (int $a, int $b, int $c): void {});
$emitter->emit('event', [1, 2, 3]);
}
/**
* @Revs(3)
* @Iterations(3)
*/
public function benchRemoveListenerOnce()
{
$emitter = new EventEmitter();
$listeners = [];
for ($i = 0; $i < 100000; $i++) {
$listeners[] = static function (int $a, int $b, int $c): void {};
}
foreach ($listeners as $listener) {
$emitter->once('event', $listener);
}
foreach ($listeners as $listener) {
$emitter->removeListener('event', $listener);
}
}
}This pull request,
|
igorw#88 - add ramsey/composer-install - fix docblocks - add import for count function - updated unit test preferring 'self':: over 'this->' for assertSame/assertCount
No worries, thanks for running those. Looks like no percentile big changes. 🎉 OS doesn't matter, as long as there isn't an increase in runtime for the introduced changes. |
😄 Also, my apologies, I may have been a bit overzealous in marking some of the conversations as resolved, now that I look back on it. Though I believe 4bd2aeb covers everything with the exception of callable signatures. |
No worries, I'll have a look at everything this weekend 👍 |
Sounds good 👍 |
Just FYI, I haven't forgotten about this, been having "fun" figuring out the |
I feel that, haha. I wracked my brain on it a bit, but kept coming back to how I have it now. Since any callable can be passed, theoretically, it's hard to really get specific with the template type it seems. |
|
Just popping in, to see where we are at. :) |
Actively working on fixing the callable argument typing: reactphp-parallel/contracts#9 |
I've been brainstorming this as well, and I see we've had similar ideas. I haven't quite nailed it yet, though. |
From what I've dug up today, there is no perfect solution. There is currently no way in PHPStan to make typing arguments work on intentionally infinite arguments for a function. (There is an open issue for it: phpstan/phpstan#8214.) I spent around 75% of the time working on that PR today just with the types tests. Those are worth it to add and get everything super clear. |
Hmm. In this instance, perhaps we could move forward with |
I'll do a follow up after this PR to address this before releasing so it won't block this PR. I'll also reach out to Igor after I created the |
There was a problem hiding this comment.
Looking great, thanks for the effort. I don't want to hold this one up any longer. For follow up PR's:
- The exact callable typing
- PHPStan type testing (https://blog.wyrihaximus.net/2024/06/updating-php-packages-to-reactphp-promise-v3--and-test-your-types-with-phpstan/)
The goal of this PR is to update Evenement to increase the requirement to PHP 8 (specifically 8.1); in reference to #87
This is my initial pass through. All tests are passing on my fork/branch, though obviously it will fail here for PHP 7.0 - 8.0.
Also, of course this would require a major version bump (
4.0.0), though I did not edit the CHANGELOG.Why PHP 8.1
Notable Changes
InvalidArgumentExceptionnow checks for empty string instead of null.event name must not be nulltoevent name must not be an empty stringemptyconstruct with a more strict comparison. E.g.if ($listeners !== []) {