Symfony 5 support #97
Conversation
With BC compatibility I hope
|
I'm a bit unsure what to do with the Failure on 4.2 which seems to be the odd one out of the bunch, if someone has a suggestion I'd like to hear it. |
|
|
||
| private function dispatch($eventName, $event) | ||
| { | ||
| if (get_class($this->eventDispatcher) === 'Symfony\\Contracts\\EventDispatcher\\EventDispatcherInterface') { |
There was a problem hiding this comment.
You should use here instanceof instead of get class. There are multiple types of event dispatchers
There was a problem hiding this comment.
Ah my bad I somehow had it stuck in my mind that that only works with the ::class constant, which doesn't exist on PHP 5.4, so I went with that instead
Tests/TestCase.php
Outdated
| @@ -2,6 +2,18 @@ | |||
|
|
|||
| namespace Noxlogic\RateLimitBundle\Tests; | |||
|
|
|||
| class TestCase extends \PHPUnit_Framework_TestCase | |||
| { | |||
| if (!class_exists('\\PHPUnit\\Framework\\TestCase')) { | |||
There was a problem hiding this comment.
Can't we just drop old phpunit? Anyway is an dev dependency
There was a problem hiding this comment.
same problem as above I guess, the need for php 5.x kinda locks you into the old phpunit...
There was a problem hiding this comment.
I think we need to create a new major version if we want to drop 5.x support?
There was a problem hiding this comment.
I mean I could move it to PHP 7.2 (since phpunit 7 will reach End of life next week)
There was a problem hiding this comment.
A major is not required here since php unit is a dev dependency, users are not impacted by it at all.
Consider also this post by the doctrine team that explains why dropping old versions is not considered a BC brak
https://www.doctrine-project.org/2017/07/25/php-7.1-requirement-and-composer.html
There was a problem hiding this comment.
Not sure if that counts as major version upgrade though
There was a problem hiding this comment.
Tbh at that point I'd go as far as limiting it to >= 4.3 since 4.2 can't seem to decide which event/dispatcher systems it wants.
There was a problem hiding this comment.
4.2 is not supported anymore and upgrading to 4.4 should be trivial so for me is a big +1
There was a problem hiding this comment.
If was up to me will do 3.4, 4.4 and 5.0
There was a problem hiding this comment.
I made it 3.4 and >= 4.3 only and now the only things that fail are the tests flagged prefer-lowest and the SF5 one (cause the oauth server bundle isn't there yet) so allowed those to fail for now
Doesn't support void returns
There was a problem hiding this comment.
3.4 is still maintained, and is a LTS, https://symfony.com/releases/3.4 why dropping it?
|
I wanted to see if the 4.3 framework still worked with the 3.4 version since it allows 3.4 in its composer file, turns out it does not work. (tried to get rid of the failures on put it back in though and now tests for 3.4 are fine |
|
LGMT - Shall I go ahead and make a new release - I think it can be a minor version bump - the composer.json requires will limit its install to people that need it - I dont see any breaking changes for the user |
|
Looks good 👍 |
|
Looking forward to seeing this merged! :) |
|
@jaytaph ok now that it's live and I ran it against production code tests it breaks.... |
|
Actually I made a PR that fixes it see #100 |
Added Symfony 5 support and also implemented contracts events (with BC)