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 remove Upload type from setUpload for Injections #11337

Closed
wants to merge 1 commit into from

Conversation

TheRealAjay
Copy link

Description

When Injecting "SilverStripe\Assets\Upload" with a custom class, the UploadReceiver.php trait throws an "Uncaught TypeError", at setUpload().

Issues

This is happening due to the "Upload" type defined in docblocks and params. Removing this type allows the Upload Class to be properly injected.

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • The commit messages follow our commit message guidelines
    • I have not used the correct commit message type as I have not checked before creating the PR
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

When Injecting "SilverStripe\Assets\Upload" with a custom class, the UploadReceiver.php trait throws an "Uncaught TypeError", at setUpload(). This is happening due to the "Upload" type defined in docblocks and params. Removing this type allows the Upload Class to be properly injected.
@GuySartorelli
Copy link
Member

Thank you for submitting this - however, the typehint is there intentionally.

Perhaps in a major release we could replace it with an interface that your custom class can implement - however you should raise an issue about that so that it can be discussed.

For now I'll close this pull request.

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.

2 participants