Skip to content

Improve _getUpload, remove getUploadedFileType method#30

Open
amulet1 wants to merge 1 commit into
horde:FRAMEWORK_6_0from
horde6:refactor_image
Open

Improve _getUpload, remove getUploadedFileType method#30
amulet1 wants to merge 1 commit into
horde:FRAMEWORK_6_0from
horde6:refactor_image

Conversation

@amulet1
Copy link
Copy Markdown
Member

@amulet1 amulet1 commented May 10, 2026

  • Separate upload handling from try/catch: file-not-uploaded logic
  • Build $img as a local array and assign to $this->_img at the end, rather than mutating $this->_img piecemeal throughout the method
  • Remove getUploadedFileType static method, inline/improve its logic
  • Related code improvements

- Separate upload handling from try/catch: file-not-uploaded logic
- Build $img as a local array and assign to $this->_img at the end,
  rather than mutating $this->_img piecemeal throughout the method
- Remove getUploadedFileType static method, inline/improve its logic
- Related code improvements
@amulet1 amulet1 requested a review from TDannhauer May 10, 2026 20:37
Copy link
Copy Markdown
Contributor

@TDannhauer TDannhauer left a comment

Choose a reason for hiding this comment

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

Looks like a solid cleanup: clearer success vs. non-upload paths, single assignment into $this->_img['img'], and MIME handling after the file is moved to temp storage.

BC: getUploadedFileType was removed as a public static method. Is that intentional for FRAMEWORK_6_0? If anything outside this package still calls it, we should either keep a deprecated stub or document the break in the changelog / upgrade notes.

Tests: Could you add or extend tests for this flow (happy path upload, no new file with existing hash, explicit remove)? Even a small unit/integration test would help lock in behavior after this refactor.

@TDannhauer TDannhauer requested a review from ralflang May 11, 2026 10:59
@TDannhauer
Copy link
Copy Markdown
Contributor

@ralflang what do you think about the BC break. is it okay?

Comment thread lib/Horde/Form/Type.php
Comment thread lib/Horde/Form/Type.php
@amulet1
Copy link
Copy Markdown
Member Author

amulet1 commented May 11, 2026

Note, the getUploadedFileType() was removed after I confirmed that it is not being used any other Horde modules.

In general, other places should rely on getImage() or getInfo() - and it looks like they do.

@amulet1 amulet1 requested a review from ralflang May 13, 2026 17:40
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