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

Fix session usage #77

Merged
merged 2 commits into from Feb 22, 2021
Merged

Fix session usage #77

merged 2 commits into from Feb 22, 2021

Conversation

fritzmg
Copy link
Sponsor Collaborator

@fritzmg fritzmg commented Feb 20, 2021

Fixes #63

The point of WidgetHelper::addFilesToSession was (presumably) to make Contao\Form::processFormData behave in the same way as with the regular file upload. i.e. if email sending is enabled, Contao will either attach the uploaded file to the email or append the absolute path to the image for easy access/download (when file storing is enabled).

However, this is currently not the case with the FineUploader. Any uploaded files only show up in the regular email content, with field name: relative/file/path. Uploaded files are not attached, nor is a direct link appended to the email by Contao.

This is because WidgetHelper::addFilesToSession uses Symfony's Session service to write the information to the session. However, Symfony's Session stores all session data always in "session bags" with unique namespaces (see Session Data Management), not directly to $_SESSION, in order to not interfere with other session data. Contao\Form::processFormData however uses $_SESSION['FILES'] directly, without Symfony's Session service - thus this never worked.

This PR fixes that by instead writing the session data directly to $_SESSION['FILES'] as expected from Contao\Form::processFormData.

Note: the Symfony Session is still added via Dependency Injection to the WidgetHelper, even though nothing in it uses it any more. Should I remove it?

@fritzmg fritzmg added the bug label Feb 20, 2021
@fritzmg fritzmg self-assigned this Feb 20, 2021
@qzminski
Copy link
Member

Sounds logical to me. Please remove the session dependency and I will merge this 😉

@qzminski qzminski merged commit 13408e5 into terminal42:main Feb 22, 2021
@qzminski
Copy link
Member

Released as 1.3.7, thank you @fritzmg !

@tsarma
Copy link

tsarma commented May 23, 2023

Why is this fix got removed from version 3.3.3 to 3.4.*?

Related issues:
#93
#91
#75

@fritzmg fritzmg deleted the fix-session-usage branch May 23, 2023 08:52
@fritzmg
Copy link
Sponsor Collaborator Author

fritzmg commented May 23, 2023

Why is this fix got removed from version 3.3.3 to 3.4.*?

Likely due to its incompatibility with Contao 5.

@tsarma
Copy link

tsarma commented May 23, 2023

Oh ok. But unfortunate to remove it so early.

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

Successfully merging this pull request may close these issues.

Missing file links
3 participants