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

Rename uploaded files using webform filename pattern (and tokens) #1009

Merged
merged 3 commits into from
Sep 28, 2024

Conversation

sluc23
Copy link
Contributor

@sluc23 sluc23 commented Sep 17, 2024

Overview

Webform hast the feature of setting specific filenames for File type fields. This allows to rename in the server uploaded files with specific names or using tokens. Description here
This feature does not work in Webform CiviCRM, the uploaded files are stored in with the original filename.. why? because how processes are executed in this order:

  1. On webform submission (or before if uploads are inline), the file is uploaded to the server, usually in the private:// location with original filename
  2. webform_civicrm handler is executed first and and copies the filename from the first private location to files/civicrm/custom with the original name.
    ref: https://github.com/colemanw/webform_civicrm/blob/6.2.5/src/WebformCivicrmBase.php#L782
  3. After that the webform code is executed and moves the file from private to final destination and applies the renaming (and replacing tokens if needed).
    ref: https://git.drupalcode.org/project/webform/-/blob/6.2.2/src/Plugin/WebformElement/WebformManagedFileBase.php?ref_type=tags#L1217
    This is where webform calculates the final filename based on this pattern:
    ref: https://git.drupalcode.org/project/webform/-/blob/6.2.2/src/Plugin/WebformElement/WebformManagedFileBase.php?ref_type=tags#L1249

So we need this feature to work in webform_civicrm, and after some research I found this solution

Before

Uploaded files are always saved with the original local name

After

Uploaded files are saved, following the filename pattern defined in the field settings

Technical Details

This is a solution I came up with, it could be better alternatives, but basically I had to copy how to calculate the filename from filename pattern in webform module.
CivicrmWebformHandler needs to have a reference to the webform.token_manager service to do the calculation of filename.. this could be beneficial for using Drupal tokens in other sections of the module's handler

Comments

Open to suggestions

@KarinG
Copy link
Collaborator

KarinG commented Sep 18, 2024

Test fails are unrelated. I'll pull in the PR and test it this week. Thank you @sluc23 👍

@jitendrapurohit
Copy link
Collaborator

Yes i think saving files in the presave sounds fine to me borrowing few code from webform module. But i think the approach can be made better if we can do the following? Eg

  • Add a custom service for civicrm file managerment, eg CivicrmFileManager.
  • Inject file_system and token manager service to it.
  • Move saveDrupalFileToCivi() to it?

Thoughts @sluc23 ?

@sluc23
Copy link
Contributor Author

sluc23 commented Sep 23, 2024

@jitendrapurohit I think it is a good idea too.. but in that case I think that refactor is much complex that what I'm able to achieve.. that's a bigger architecture modification. Creating a new service in the module will require more tests, check that does not affect other sections of the module source code, taking important decisions, etc..

In the case you want to move to that direction i can offer my self to review/test the solution, but not to code it..

@KarinG
Copy link
Collaborator

KarinG commented Sep 23, 2024

I'm ok with the simpler approach here (for now). Thank you @sluc23

@jitendrapurohit did you have a chance the functionality? If yes - then when can merge this.

@jitendrapurohit
Copy link
Collaborator

yes have tested and it works as per the functionality is concerned. Merging. Thanks @sluc23 👍

@jitendrapurohit jitendrapurohit merged commit 5e185dd into colemanw:6.x Sep 28, 2024
3 of 4 checks passed
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