nette / forms

📝 Generating, validating and processing secure forms in PHP. Handy API, fully customizable, server & client side validation and mature design.
https://doc.nette.org/forms
Other
497 stars 147 forks source link

BC break: narrowed return type of UploadControl in 3.2.4 #338

Closed jankonas closed 1 week ago

jankonas commented 2 months ago

Version: 3.2.4

The return type of \Nette\Forms\Controls\UploadControl::getValue() was narrowed to FileUpload|array|null in 3.2.4 which breaks compatibility with custom controls extending UploadControl.

If the custom control returns something else than FileUpload or it's return type is mixed, PHP throws an error:

PHP Fatal error:  Declaration of CustomControl::getValue(): ... must be compatible with Nette\Forms\Controls\UploadControl::getValue(): Nette\Http\FileUpload|array|null

Could you please widen the return type again or is extending UploadControl not supported?

dg commented 2 months ago

This is, of course, an unintended BC break. Couldn't you fix your CustomControl?

jankonas commented 2 months ago

For now I "fixed it" by declaring conflict with nette/forms 3.2.4 in composer.json.

I don't think I have any option to fix it in other way then duplicate code from UploadControl instead of extending it, because the FileUpload is final so I can't make the return type to exted FileUpload.

The use case is that I have a javascript image cropper which uploads image with other data for cropping (coordinates, width and height) and the control adds this data to the value:

class CroppedImageControl extends UploadControl
{
    private ?CropperImageOptions $cropperOptions = null;

    public function __construct(?string $label = null)
    {
        parent::__construct($label);
        $this->monitor(Form::class, function (Form $form): void {
            $form->addHidden($this->name . 'CropperX')->setOmitted();
            $form->addHidden($this->name . 'CropperY')->setOmitted();
            $form->addHidden($this->name . 'CropperHeight')->setOmitted();
            $form->addHidden($this->name . 'CropperWidth')->setOmitted();
        });
    }

    public function getValue(): CroppedImage
    {
        /** @var FileUpload $file */
        $file = $this->value;

        /** @var array<string, string> $values */
        $values = (array) $this->form->getHttpData();

        $name = $this->getName() ?? '';

        return new CroppedImage(
            $file,
            (float) $values[$name . 'CropperX'],
            (float) $values[$name . 'CropperY'],
            (float) $values[$name . 'CropperHeight'],
            (float) $values[$name . 'CropperWidth'],
            $this->cropperOptions ?? new CropperImageOptions()
        );
    }

    public function setOptions(int $viewMode, string $dragMode, ?string $aspectRatio = null, ?string $data = null): void
    {
        $this->cropperOptions = new CropperImageOptions($viewMode, $dragMode, $aspectRatio, $data);
    }
}
dg commented 2 months ago

Of course duplicate the code, there's nothing wrong with that. It avoids a lot of problems.

jankonas commented 2 months ago

OK, just to confirm I understand you correctly, you want to keep the narrowed return type in the 3.x versions?

dg commented 1 week ago

Yes. I understand that it broke your code, but using inheritance was wrong in this case, it violated LSP. So the correct solution is to get rid of inheritance and that will solve the problem.