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.

Sign up for GitHub

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

Jump to bottom

ENH Add support for uploading multiple files to the file upload field #1367

Open
wants to merge 1 commit into
base: 6
Choose a base branch
from
Open

ENH Add support for uploading multiple files to the file upload field #1367

wants to merge 1 commit into from

Conversation

rasstislav
Copy link
Contributor

@rasstislav rasstislav commented Feb 6, 2025 •
edited
Loading

Issues

Pull request checklist

  • The target branch is correct
  • 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 GuySartorelli mentioned this pull request Feb 7, 2025
Open
@rasstislav rasstislav force-pushed the feat-upload-multiple-files branch 3 times, most recently from faad61f to adee684 Compare February 7, 2025 08:19
@rasstislav rasstislav force-pushed the feat-upload-multiple-files branch from adee684 to 5454e25 Compare February 7, 2025 21:41
@GuySartorelli
Copy link
Member

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

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
GuySartorelli requested changes Feb 17, 2025
View reviewed changes
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

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.

code/Control/UserDefinedFormController.php
if (!empty($_FILES[$field->Name]['name'])) {
if (($names = $_FILES[$field->Name]['name'] ?? '') && (!is_array($names) || array_filter($names))) {
Copy link
Member

Choose a reason for hiding this comment

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

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

code/Control/UserDefinedFormController.php
$validationResult = $e->getResult();
foreach ($validationResult->getMessages() as $message) {
$form->sessionMessage($message['message'], ValidationResult::TYPE_ERROR);
$processFile = function ($fileData) use ($field, $form, &$attachments) {
Copy link
Member

Choose a reason for hiding this comment

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

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

code/Model/EditableFormField/EditableFileField.php
@@ -40,6 +42,7 @@ class EditableFileField extends EditableFormField
private static $db = [
'MaxFileSizeMB' => 'Float',
'FolderConfirmed' => 'Boolean',
'Multiple' => 'Boolean',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Multiple' => 'Boolean',
'IsMultiple' => 'Boolean',

code/Model/EditableFormField/EditableFileField.php
@@ -209,7 +218,7 @@ public function createProtectedFolder(): void

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

Choose a reason for hiding this comment

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

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

code/Model/EditableFormField/EditableFileField.php
@@ -240,6 +253,8 @@ public function getFormField()

$this->doUpdateFormField($field);

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

Choose a reason for hiding this comment

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

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

code/Model/EditableFormField/EditableFileField.php
@@ -240,6 +253,8 @@ public function getFormField()

$this->doUpdateFormField($field);

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

Choose a reason for hiding this comment

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

Where is this used?

code/Model/Submission/SubmittedFileField.php
Comment on lines 24 to +30
private static $has_one = [
'UploadedFile' => File::class
];

private static $many_many = [
'UploadedFiles' => File::class
];
Copy link
Member

Choose a reason for hiding this comment

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

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

code/Model/Submission/SubmittedFileField.php
Comment on lines -96 to +120
* @return string
* @return array|null
*/
public function getLink($grant = true)
public function getLinks($grant = true)
Copy link
Member

Choose a reason for hiding this comment

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

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.

code/Model/Submission/SubmittedFileField.php
*/
public function getUploadedFileFromDraft()
public function getUploadedFilesFromDraft()
Copy link
Member

Choose a reason for hiding this comment

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

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.

templates/SilverStripe/UserForms/Model/EditableFormField/EditableFileField.ss
@@ -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 %>/>
Copy link
Member

Choose a reason for hiding this comment

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

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 %>/>

Copy link
Member

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuySartorelli GuySartorelli GuySartorelli requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rasstislav @GuySartorelli