Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FileElement: Preserve file across requests #107

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

nilmerg
Copy link
Member

@nilmerg nilmerg commented Jan 24, 2023

No description provided.

@nilmerg nilmerg self-assigned this Jan 24, 2023
@cla-bot cla-bot bot added the cla/signed label Jan 24, 2023
@nilmerg nilmerg force-pushed the preserve-chosen-file-element-value branch 4 times, most recently from 49b2024 to b27e10f Compare January 27, 2023 10:49
@nilmerg nilmerg marked this pull request as ready for review January 27, 2023 10:59
@nilmerg nilmerg force-pushed the preserve-chosen-file-element-value branch from b27e10f to da911b9 Compare January 27, 2023 10:59
@nilmerg nilmerg requested a review from yhabteab January 27, 2023 11:00
@nilmerg
Copy link
Member Author

nilmerg commented Jan 27, 2023

@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 Icinga/ipl-web#112 or Icinga/icingaweb2#4990.

@yhabteab
Copy link
Member

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
Copy link
Member Author

nilmerg commented Jan 30, 2023

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.

Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a file element without the destination attribute, upload images, and auto-submit the form. After that, the file element will look like this.

Bildschirm­foto 2023-01-30 um 11 35 24

@nilmerg nilmerg force-pushed the preserve-chosen-file-element-value branch from da911b9 to f4172ff Compare January 30, 2023 10:54
@nilmerg nilmerg requested a review from yhabteab January 30, 2023 10:55
@yhabteab
Copy link
Member

yhabteab commented Feb 1, 2023

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
Copy link
Member Author

nilmerg commented Feb 1, 2023

I can't reproduce that.

@yhabteab
Copy link
Member

yhabteab commented Feb 1, 2023

I can't reproduce that.

Sorry! It's a reporting module bug.

yhabteab
yhabteab previously approved these changes Feb 1, 2023
Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works!

@nilmerg nilmerg force-pushed the preserve-chosen-file-element-value branch from f4172ff to bce56f0 Compare February 1, 2023 12:37
@nilmerg
Copy link
Member Author

nilmerg commented Feb 1, 2023

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

@yhabteab
Copy link
Member

yhabteab commented Feb 1, 2023

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
Copy link
Member Author

nilmerg commented Feb 1, 2023

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? 😅

@yhabteab
Copy link
Member

yhabteab commented Feb 1, 2023

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

@nilmerg nilmerg merged commit 64ca02d into master Feb 2, 2023
@nilmerg nilmerg deleted the preserve-chosen-file-element-value branch February 2, 2023 10:19
@lippserd
Copy link
Member

lippserd commented Feb 2, 2023

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
Copy link
Member Author

nilmerg commented Feb 2, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants