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

[piridium]

Bug Report

Current Behavior
Update fof/upload from 1.5.8 to 1.6.0 throws the following deprectation error:

Deprecated: Optional parameter $upload declared before required parameter $contents is implicitly treated as a required parameter in ./vendor/fof/upload/src/Adapters/Flysystem.php on line 55

Environment

• Flarum version: 1.8.9
• Extension version: ^1.6.0
• Webserver: apache
• Hosting environment: local development server
• PHP version: 8.2.25

Possible solution(s)
Switching off deprecation warnings is not a good idea because
a) I want to have them enabled on the dev server, and
b) we should fix deprecations in order to not run into any problems in the future.

As of php 8.0 having an optional parameter followed by a required one is deprecated. (https://www.php.net/manual/en/migration80.deprecated.php)
It is recommended to use a explicit nullable type instead.

Suggestion:

# change line 55 from
public function upload(File $file, ?UploadedFile $upload = null, $contents)
# into
public function upload(File $file, ?UploadedFile $upload, $contents)

This gets rid of the error. And as UploadedFile is explicitly nullable, it should be ok, right? This is where I am not sure if we will run into any problems if a caller does not specify UploadedFile.

Additional Context
Duplicate of #407 which has already been closed.

[piridium]

This is still preventing us from upgrading to 1.6.0+. How is everybody else doing it? Did you guys all deactivate deprecation warnings?!

[Nexulo]

This is still preventing us from upgrading to 1.6.0+. How is everybody else doing it? Did you guys all deactivate deprecation warnings?!

I can only speak for myself and of course I see this warning message in the logs. But the application still works as it should, it is “only” a "PHP Deprecated" message and should be fixed.

[piridium]

The problem is not the deprecation warning, but the fatal error which followed in its wake.

However, I just saw a strange behavior: I disabled error warnings in the php.ini display_errors = Off and the error was gone, so far as expected. I then re-enabled errors display_errors = On and the error did not come back.

So I am now able to upgrade to 1.6.0+. 😄

[Nexulo]

There should not be triggered a error because of the deprecated message. There will be other reasons for this, because deprecated means that this “feature” will soon be removed but is currently still working. If you display these messages directly on the website instead of logging them in a log file, this can lead to following errors. Or am I misunderstanding something?

[piridium]

This is also how I understand deprecation warnings. They should not actually trigger any further errors. I somehow perceived it that way because the same two errors were reported in the linked issue #407 .
However, turning the error messages off and on again somehow inexplicably eliminated both.

In any case, we should solve this deprecation. I think we could do it as I suggested above: $upload is already marked as nullable, so we don't need to pass a default value.