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

UserFormFileExtension #964

Merged

Conversation

emteknetnz
Copy link
Member

Extension method implementation to check if a file is a userform file upload

Related PR: silverstripe/silverstripe-assets#398

Issue silverstripe/silverstripe-asset-admin#1066

Copy link
Member

@Cheddam Cheddam left a comment

Choose a reason for hiding this comment

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

A couple of suggestions, but otherwise looking good. I've recommended an adjustment to the naming of the new API this relies on, so I'll hold off on approving this until that's resolved and this PR is updated (if necessary).

code/Extension/UserFormFileExtension.php Outdated Show resolved Hide resolved
code/Extension/UserFormFileExtension.php Outdated Show resolved Hide resolved
code/Extension/UserFormFileExtension.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/5/file-manager-icons branch 4 times, most recently from 1942d18 to dc2c157 Compare May 21, 2020 06:56
code/Extension/UserFormFileExtension.php Show resolved Hide resolved
code/Extension/UserFormFileExtension.php Show resolved Hide resolved
tests/Extension/UserFormFileExtensionTest.php Outdated Show resolved Hide resolved
tests/Extension/UserFormFileExtensionTest.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/5/file-manager-icons branch from dc2c157 to 37e6439 Compare May 21, 2020 21:54
@dhensby
Copy link
Contributor

dhensby commented May 23, 2020

Something about this approach feels off. Why aren't we using the pre-existing relationship between SubmittedFileField and File to determine if they belong to a UserForm?

if ($file->UploadField()->exists()) {
   return true;
}

All this would need is to add a config value of:

SilverStripe\Assets\File:
  belongs_to:
    UploadField: SilverStripe\UserForms\Model\Submission\SubmittedFileField

@emteknetnz emteknetnz force-pushed the pulls/5/file-manager-icons branch 2 times, most recently from 5835597 to e6ee32d Compare May 24, 2020 22:18
@emteknetnz
Copy link
Member Author

@dhensby have updated to use $file->SubmittedFileField()->exists()

@emteknetnz emteknetnz force-pushed the pulls/5/file-manager-icons branch 6 times, most recently from 2797dfd to dbbb02a Compare May 25, 2020 03:24
Copy link
Member

@Cheddam Cheddam left a comment

Choose a reason for hiding this comment

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

I've provided some additional thoughts on Dan's outstanding feedback, but basically I think this is good to go now.

@emteknetnz Take a quick look at the second one and see if you agree with my suggestion, otherwise let's merge.

code/Extension/UserFormFileExtension.php Show resolved Hide resolved
code/Extension/UserFormFileExtension.php Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/5/file-manager-icons branch from dbbb02a to 2d3f95c Compare May 28, 2020 01:09
- UserFormUpload used by File::isTrackedFormUpload()
@emteknetnz emteknetnz force-pushed the pulls/5/file-manager-icons branch from 2d3f95c to ed53709 Compare May 28, 2020 01:14
@emteknetnz emteknetnz merged commit 447be10 into silverstripe:5 May 28, 2020
@dhensby
Copy link
Contributor

dhensby commented Jun 17, 2020

My overall point here is why are we adding a new field that de-normalises this information when it can be worked out from existing database relationships.

We know which files were uploaded from formfields by seeing if there is a relationship with an EditableFormField.

We can replace all this entire PR with:

public function getUserFormUpload() {
    return $value = $file->SubmittedFileField()->exists();
}

@dhensby
Copy link
Contributor

dhensby commented Jun 17, 2020

The hardcoded column name isn't ideal, perhaps this could be a class constant that's used in the $db array? Otherwise I think this can move forward as-is for now.

The correct way to do it would be to use DataObjectSchema

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

Successfully merging this pull request may close these issues.

3 participants