search

Vinelab / bowler

RabbitMQ wrapper for Laravel
MIT License
45 stars 9 forks source link

Laravel 6 compatibility #57

Closed specialtactics closed 5 years ago

specialtactics commented 5 years ago

Make package compatible with Laravel 6 Also bump min php version from 7.0 to 7.1

I checked, and none of the changes affect backwards versions. The only "breaking" change is bumping the PHP vesion, but PHP 7.0 (and in fact even 7.1) hasn't been supported for a while.

One note I wanted to raise, the use of assertAttributeEquals is deprecated in phpunit 8 and will be removed in 9. It has no direct replacement, but I think the thing to do is just to add getter methods to the Connection class, which will allow testing. Or alternatively, creating a testing double that extends Connection and has those getter methods. I would be happy to implement either option, just let me know which you think is best.

KinaneD commented 5 years ago

Sure, eventually we'd need to move away form that assertion. What is you'r take on it? Does providing getters in Connection::class imply any risks? See this discussion for further insights on the deprecation and alternatives https://github.com/sebastianbergmann/phpunit/issues/3339. We can follow up on the discussion in the referenced issue. Thanks for the contribution.

specialtactics commented 5 years ago

Hey @KinaneD thanks for the quick merge and tag !

With regarding to phpunit, thanks for linking that - haven't seen it before. I don't have any opinion, I think mocking the connection class as a test double/fixture just for testing won't have any repercussions if you don't want those private attributes to be readable.

However if it doesn't make any difference (I am not too familiar with this package so I can't make this call), it would be easier just to add getters directly to the connection class. The difference here of course between making them public is that they will be read-only from an external perspective.

KinaneD commented 5 years ago

Sure, I don't think its is an issue to add getters here, since these attributes are provided by the application.

specialtactics commented 5 years ago

No problem @KinaneD I will do that.