Icinga / ipl-html

HTML abstraction layer for the Icinga PHP Library
MIT License
1 stars 1 forks source link

FileElement: Preserve file across requests #107

Closed nilmerg closed 1 year ago

nilmerg commented 1 year ago

@yhabteab An example usage I tested this with is:

        $form = new class extends CompatForm {
            protected function assemble()
            {
                $this->setAttribute('enctype', 'multipart/form-data');
                $this->addElement('checkbox', 'checkbox', [
                    'class' => 'autosubmit',
                    'label' => 'A decision'
                ]);
                $this->addElement('text', 'text', [
                    'label' => 'Some text'
                ]);
                $this->addElement('file', 'file', [
                    'label' => 'A file',
                    'description' => 'Really, it is a file',
                    'accept' => ['image/*', '.png', '.jpg'],
                    'capture' => 'user',
                    'destination' => sys_get_temp_dir(),
                    'multiple' => true
                ]);
                $this->addElement('submit', 'submit', [
                    'label' => 'Submit'
                ]);
            }
        };

        $form->handleRequest($this->getServerRequest());

        if (($el = $form->getElement('file'))->hasValue()) {
            var_dump($el->getValue());
        }

        $this->addContent($form);

You also need https://github.com/Icinga/ipl-web/pull/112 or https://github.com/Icinga/icingaweb2/pull/4990.

yhabteab commented 1 year ago

I think 🤔 I have found a bug.

  1. Upload an image
  2. Autosubmit the form with another form element
  3. You can no longer upload a second image for the same element
nilmerg commented 1 year ago

No, that's intended. The same happens if you don't (auto)submit and only choose a single file again. This also overrides any and all previously set files. To upload different or more files the user needs to remove the uploaded files first.

yhabteab commented 1 year ago

Create a file element with/without the destination attribute, upload images, and auto-submit the form twice. Bildschirm­foto 2023-02-01 um 10 00 42

nilmerg commented 1 year ago

I can't reproduce that.

yhabteab commented 1 year ago

I can't reproduce that.

Sorry! It's a reporting module bug.

nilmerg commented 1 year ago

@yhabteab I've just removed the possibility to populate the element with anything else than instances of UploadedFileInterface.

yhabteab commented 1 year ago

Shouldn't the element initiate a stream for the uploaded file(s) as well? At the moment the $file->getStream() method is returning null in an on success handler trying to save the image to the database.

nilmerg commented 1 year ago

It's not done explicitly by the element, but lazily by the underlying library. This also should work fine, the tests for it succeed. Personally, I can't reproduce it as well. Is it maybe an issue in the reporting module again? :sweat_smile:

yhabteab commented 1 year ago

Is it maybe an issue in the reporting module again?

Indeed! But it's my fault 🙈. LGTM! Bildschirm­foto 2023-02-01 um 15 59 10

lippserd commented 1 year ago

Shouldn't sys_get_temp_dir() be the default? My understanding is, that I need destination in oder to have the files preserved across request. Is that correct?

nilmerg commented 1 year ago

Yeeh, your questions come always right in time!

Is that correct?

Yes. It's configurable because e.g. Icinga Web uses it's own temp directory. Making sys_get_temp_dir the default is of course an option.