search

laravel / octane

Supercharge your Laravel application's performance.
https://laravel.com/docs/octane
MIT License
3.77k stars 296 forks source link

[2.x] Prefix FrankenPHP version with `v` for `version_compare()` #776

Closed cwhite92 closed 10 months ago

cwhite92 commented 10 months ago

I was experimenting with creating a Docker image to run a FrankenPHP web server with Octane. I made the following Dockerfile:

FROM dunglas/frankenphp

RUN install-php-extensions \
    pcntl

COPY . /app

ENTRYPOINT ["php", "artisan", "octane:frankenphp"]

When I started the container it immediately exited, and when inspecting the logs I found that Octane was attempting to download the FrankenPHP binary despite it already existing in the Docker image at /usr/local/bin/frankenphp:

   WARN  Your FrankenPHP binary version (v1.0.1) may be incompatible with Octane.

 Should Octane download the latest FrankenPHP binary version for your operating system? (yes/no) [yes]:
 > 
   ERROR  FrankenPHP binaries are currently only available for Linux (x86_64) and macOS. Other systems should use the Docker images or compile FrankenPHP manually.

   INFO  Server running…

  Local: http://127.0.0.1:8000 

  Press Ctrl+C to stop the server

   INFO  sh: 1: exec: /usr/local/bin/frankenphp: not found

After some debugging I found that the version_compare() on this line might not be working the way we think. The frankenphp binary returns v1.0.1 as its version and it's being compared to 1.0.0 without the v. As you can see here v1.0.1 >= 1.0.0 returns false, and causes Octane to re-download the binary: https://3v4l.org/uWjgX

After changing $requiredFrankenPhpVersion to v1.0.1 in my local files and starting the container again, Octane doesn't try to re-download the frankenphp binary and the container runs successfully.

driesvints commented 10 months ago

We don't want to make the adjustment in the property here. We always define the version in source code for our packages without it: https://github.com/laravel/cashier-paddle/blob/c3b70784918ebecb12b28cda1b04aabe39e0ca8b/src/Cashier.php#L16

The correct fix is to adjust this in the place where it's used.

nunomaduro commented 10 months ago

Currently re-working on this. Thank you for this report.