ENH Add support for uploading multiple files to the file upload field by rasstislav · Pull Request #1367 · silverstripe/silverstripe-userforms

rasstislav · 2025-02-06T18:15:18Z

Issues

Pull request checklist

The target branch is correct
- See picking the right version
All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
- Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
The commit messages follow our commit message guidelines
The PR follows our contribution guidelines
Code changes follow our coding conventions
This change is covered with tests (or tests aren't necessary for this change)
Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
CI is green

GuySartorelli · 2025-02-17T03:22:59Z

In the future please do not remove sections from the pull request template - having a quick description and manual testing steps in the PR description goes a long way to making reviews easier and faster, and therefore more likely to happen.

GuySartorelli · 2025-02-17T03:26:17Z

You haven't ticked the checkbox "This change is covered with tests (or tests aren't necessary for this change)" - this implies you think tests are required and haven't added them. If tests are needed, please add them.

GuySartorelli

Haven't done a thorough review, but these things stood out to me.

Please note that we'll be releasing CMS 5.4.0-beta1 soon, hopefully some time this week, and that is also the change freeze for any changes for CMS 5.

I don't think it's likely this will be approved for merging before then, so it probably makes sense to retarget this to the 7 branch with the intention of it being included in the next major release.

GuySartorelli · 2025-02-17T03:27:24Z

            if (!empty($data[$field->Name])) {
                if (in_array(EditableFileField::class, $field->getClassAncestry() ?? [])) {
-                    if (!empty($_FILES[$field->Name]['name'])) {
+                    if (($names = $_FILES[$field->Name]['name'] ?? '') && (!is_array($names) || array_filter($names))) {


Please move the variable assignment to be outside the if()

GuySartorelli · 2025-02-17T03:28:00Z

-                                $validationResult = $e->getResult();
-                                foreach ($validationResult->getMessages() as $message) {
-                                    $form->sessionMessage($message['message'], ValidationResult::TYPE_ERROR);
+                        $processFile = function ($fileData) use ($field, $form, &$attachments) {


Please move this callback to be a private method in this class instead of an anonymous function.

GuySartorelli · 2025-02-17T03:30:04Z

    private static $db = [
        'MaxFileSizeMB' => 'Float',
        'FolderConfirmed' => 'Boolean',
+        'Multiple' => 'Boolean',


Suggested change

'Multiple' => 'Boolean',

'IsMultiple' => 'Boolean',

GuySartorelli · 2025-02-17T03:31:14Z

    public function getFormField()
    {
-        $field = FileField::create($this->Name, $this->Title ?: false)
+        $field = FileField::create(($originalName = $this->Name) . ($this->Multiple ? '[]' : ''), $this->Title ?: false)


Suggested change

$field = FileField::create(($originalName = $this->Name) . ($this->Multiple ? '[]' : ''), $this->Title ?: false)

$field = FileField::create($this->Name . ($this->Multiple ? '[]' : ''), $this->Title ?: false)

GuySartorelli · 2025-02-17T03:31:26Z


        $this->doUpdateFormField($field);

+        $field->setAttribute('htmlID', $originalName);


Suggested change

$field->setAttribute('htmlID', $originalName);

$field->setAttribute('htmlID', $this->Name);

GuySartorelli · 2025-02-17T03:31:39Z


        $this->doUpdateFormField($field);

+        $field->setAttribute('htmlID', $originalName);


Where is this used?

GuySartorelli · 2025-02-17T03:34:04Z

    private static $has_one = [
        'UploadedFile' => File::class
    ];

+    private static $many_many = [
+        'UploadedFiles' => File::class
+    ];


Feels bad having both of these, but I guess we have to do this until the next major release.

GuySartorelli · 2025-02-17T03:35:50Z

-     * @return string
+     * @return array|null
     */
-    public function getLink($grant = true)
+    public function getLinks($grant = true)


We can't just rename methods outside of a major release.
Either target this to the 7 branch or implement a new method and deprecate the old one.

GuySartorelli · 2025-02-17T03:35:54Z

+     * @return array<File>|null
     */
-    public function getUploadedFileFromDraft()
+    public function getUploadedFilesFromDraft()


We can't just rename methods outside of a major release.
Either target this to the 7 branch or implement a new method and deprecate the old one.

GuySartorelli · 2025-02-17T03:36:13Z

@@ -1,2 +1,2 @@
 <input type="hidden" name="MAX_FILE_SIZE" value="$MaxFileSize" />
-<input $AttributesHTML<% if $RightTitle %> aria-describedby="{$Name}_right_title"<% end_if %>/>
+<input $getAttributesHTML(htmlID)<% if $RightTitle %> aria-describedby="{$getAttribute(htmlID)}_right_title"<% end_if %>/>


Suggested change

<input $getAttributesHTML(htmlID)<% if $RightTitle %> aria-describedby="{$getAttribute(htmlID)}_right_title"<% end_if %>/>

<input $getAttributesHTML("htmlID")<% if $RightTitle %> aria-describedby="{$getAttribute("htmlID")}_right_title"<% end_if %>/>

Please do this for all other cases where a string is being passed to a function from a template.

GuySartorelli mentioned this pull request Feb 7, 2025

Multiple images uploadfield #1242

Open

rasstislav force-pushed the feat-upload-multiple-files branch 3 times, most recently from faad61f to adee684 Compare February 7, 2025 08:19

feat: add support for uploading multiple files to the file upload field

5454e25

rasstislav force-pushed the feat-upload-multiple-files branch from adee684 to 5454e25 Compare February 7, 2025 21:41

GuySartorelli requested changes Feb 17, 2025

View reviewed changes

	$field = FileField::create(($originalName = $this->Name) . ($this->Multiple ? '[]' : ''), $this->Title ?: false)
	$field = FileField::create($this->Name . ($this->Multiple ? '[]' : ''), $this->Title ?: false)


		$this->doUpdateFormField($field);

		$field->setAttribute('htmlID', $originalName);

	$field->setAttribute('htmlID', $originalName);
	$field->setAttribute('htmlID', $this->Name);

	<input $getAttributesHTML(htmlID)<% if $RightTitle %> aria-describedby="{$getAttribute(htmlID)}_right_title"<% end_if %>/>
	<input $getAttributesHTML("htmlID")<% if $RightTitle %> aria-describedby="{$getAttribute("htmlID")}_right_title"<% end_if %>/>

Conversation

rasstislav commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Pull request checklist

Uh oh!

GuySartorelli commented Feb 17, 2025

Uh oh!

GuySartorelli commented Feb 17, 2025

Uh oh!

GuySartorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rasstislav commented Feb 6, 2025 •

edited

Loading