FriendsOfFlarum / upload

The file upload extension with insane intelligence for your Flarum forum.
https://discuss.flarum.org/d/4154
MIT License
177 stars 96 forks source link

XAMPP Optional parameter $upload declared before required parameter $contents is implicitly treated as a required parameter #407

Open Dret42 opened 1 month ago

Dret42 commented 1 month ago

Bug Report

Current Behavior When I try to upload an image I get an error and the image is not inserted inline BUT it is loaded into my image folder. Likewise, if I try to delete them from my media folder I receive an error but then the image is deleted (after refresh the page).

Steps to Reproduce

  1. Click to upload an image into a post
  2. An error return and the image is not inserted into post
  3. Go to your media folder and the image is present
  4. Try to delete the image and another error return
  5. Refresh the page and the image is deleted

Expected Behavior The image are correctly inserted and deleted without error messages.

Environment

XAMPP

Flarum 1.8.5 PHP 8.2.4 MySQL 10.4.28-MariaDB Driver Code: sync Driver Sessione: file

Browser

Chrome and Firefox

clarkwinkelmann commented 1 month ago

Please retrieve the error message from the log file(s) https://docs.flarum.org/troubleshoot

Dret42 commented 1 month ago

Here the Log file:

POST http://localhost/flarum/api/fof/upload


Deprecated: Optional parameter $upload declared before required parameter $contents is implicitly treated as a required parameter in C:\XAMPP\htdocs\flarum\vendor\fof\upload\src\Adapters\Flysystem.php on line 51

Fatal error: Uncaught Laminas\HttpHandlerRunner\Exception\EmitterException: Output has been emitted previously; cannot emit response in C:\XAMPP\htdocs\flarum\vendor\laminas\laminas-httphandlerrunner\src\Exception\EmitterException.php:20 Stack trace:

0 C:\XAMPP\htdocs\flarum\vendor\laminas\laminas-httphandlerrunner\src\Emitter\SapiEmitterTrait.php(42): Laminas\HttpHandlerRunner\Exception\EmitterException::forOutputSent()

1 C:\XAMPP\htdocs\flarum\vendor\laminas\laminas-httphandlerrunner\src\Emitter\SapiEmitter.php(21): Laminas\HttpHandlerRunner\Emitter\SapiEmitter->assertNoPreviousOutput()

2 C:\XAMPP\htdocs\flarum\vendor\laminas\laminas-httphandlerrunner\src\RequestHandlerRunner.php(75): Laminas\HttpHandlerRunner\Emitter\SapiEmitter->emit(Object(Flarum\Api\JsonApiResponse))

3 C:\XAMPP\htdocs\flarum\vendor\flarum\core\src\Http\Server.php(45): Laminas\HttpHandlerRunner\RequestHandlerRunner->run()

4 C:\XAMPP\htdocs\flarum\index.php(26): Flarum\Http\Server->listen()

5 {main}

thrown in C:\XAMPP\htdocs\flarum\vendor\laminas\laminas-httphandlerrunner\src\Exception\EmitterException.php on line 20

clarkwinkelmann commented 1 month ago

I guess the definition of that method is technically not proper PHP, but I don't even know if we can fix it without breaking backward compatibility :thinking:

https://github.com/FriendsOfFlarum/upload/blob/c5faa01648e90f10b9a98816eb67c707402f0660/src/Adapters/Flysystem.php#L51

The default parameter is not part of the contract interface though, so it won't break any compatibility in regard to class inheritance.


While we figure this out, you should be able to get rid of this error by just disabling deprecation warnings on your site, as PHP recommends doing in production. The easiest is to change display_errors to Off in your php.ini, this will also hide any PHP general warnings which you don't need in production and happen with other Flarum extensions as well.

Dret42 commented 1 month ago

I tried your workaround (display_errors = Off in php.ini) but the problem still persist. :( I'll wait for a better solution if possible.

Thanks anyway! :)

clarkwinkelmann commented 1 month ago

Make sure you edited the correct php.ini (both the CLI and webserver [Apache/nginx/fpm] if possible, but mostly the one for the webserver). If you made the change to PHP configuration, the error message will be gone or different. Also try restarting PHP, Apache, nginx or the server (depending on what your host allows you to do) after editing the configuration

Edit: I forgot this was about XAMPP. It's been a while since I used it, I don't remember if they use multiple php.ini files or just a global one. But if the error message is still the same, the change was definitely not registered

Dret42 commented 1 month ago

XAMPP has only a single php.ini file. I restarted the Apache service after every trial but has no effect: error persits!

:(