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

Filter uploaded files from hasUploads if ERR_NO_FILE detected #8085

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

Conversation

thojou
Copy link
Contributor

@thojou thojou commented Sep 18, 2024

As outlined in https://mantis.ilias.de/view.php?id=42106, we encountered an issue with file upload handling in the assFileUpload component. Specifically, when a form with a file upload field is submitted without selecting a file, it results in an empty UploadedFile object. Below is a sample dump illustrating the issue:

file upload dump

The problem arises because the hasUploads method still returns true, as it counts any available entries from the HTTP request, regardless of whether a file was actually uploaded.

We modified the hasUploads method to filter out files with the error "ERR_NO_FILE", ensuring that they are not counted as valid uploads.

If this change is likely to introduce side effects—for example, if the method is expected to return all files, regardless of their error state—an alternative solution could be to implement a custom hasValidUploads method. This would specifically count only successfully uploaded files.

@thojou thojou added the php Pull requests that update Php code label Sep 18, 2024
@thojou thojou force-pushed the bugfix/8/has-uploads branch 3 times, most recently from 0ca9a1e to d3ad598 Compare September 19, 2024 07:36
@thojou thojou force-pushed the bugfix/8/has-uploads branch from d3ad598 to cd98a4f Compare September 19, 2024 07:39
@dsstrassner
Copy link
Contributor

Hi @chfsx, any updates on this PR? I ask in my role as authority for T&A, the Mantis issue 42106 was first assigned to me. As the end of full support for ILIAS 8 is near, would it be possible to merge this PR or needs the PR some changes from @thojou?

Thanks and best regards

@dsstrassner

@kergomard
Copy link
Contributor

kergomard commented Dec 8, 2024

Hi @chfsx
I don't know if this is needed and correct. At least in the test I think the current behavior without the changes is correct and the issue in the test needed a different fix.

Best,
@kergomard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants