search

georgeboot / laravel-echo-api-gateway

Use Laravel Echo with API Gateway Websockets. Works with Bref.sh and Laravel Vapor.
MIT License
99 stars 21 forks source link

Small syntactic change which makes the code PHP 7.4 compatible #27

Closed leob closed 1 year ago

leob commented 1 year ago

Hi there - thank you for this great project, which made it a piece of cake to add websocket support to my (Bref based) AWS Lambda "serverless Laravel" project!

I just followed the (very clear and complete) docs, did a tiny bit tweaking and debugging (mainly in my frontend code), and abracadabra "it all just worked".

However I would like to propose a little change - I'm using PHP 7.4, and although I know the project states PHP 8 as the required minimum version, I saw that with a couple of very simple syntactic changes we can make it work with PHP 7.4 (I didn't want to move to PHP 8 for a number of reasons).

Point is that PHP 7.4 does not support protected in parameter declarations, so if we just move that to a property declaration then PHP 7.4 is happy, and all is well.

Tested it and all looking good, this should be a very low risk change (while I was at it I also removed an unused import).

Let me know if it looks okay, thanks in advance!

P.S. just now I saw in an Issue (https://github.com/georgeboot/laravel-echo-api-gateway/issues/10) that you'd like to 'gently encourage' people to upgrade their PHP - however, with all due respect, this stuff works equally well with PHP 7.4, and people can have pragmatic reasons to stick with 7.4 - I'd say there's no real reason here to force PHP 8 (but I have no problem with declaring PHP 8 as a requirement in composer.json, I can choose to obey or ignore that)

P.S. there's one other small change that I'd like to propose (but I will do that in a separate pull request): the frontend (Javascript) code contains some console.log statements - these are super useful, but my proposal would be that we might make this configurable (meaning we can switch it on or off) e.g. via a the options object that's passed to new Echo ... just a heads-up, I'll put the details in another PR

leob commented 1 year ago

I have no issue with getting rid of promoted contractor parameters, but then we should define the properties separately. Setting undefined properties will be deprecated in php 8.2

Right, yes I agree - however, that's exactly what I did now, or not? For instance in ConnectionRepository I declared:

protected SubscriptionRepository $subscriptionRepository;

and same story for the two properties in Handler - I added property declarations for those.

georgeboot commented 1 year ago

My bad! I was checking on GitHub mobile and didn't see it.