Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

PSR-7 UploadedFile compatibility for RenameUpload filter #70

Merged
merged 16 commits into from
Dec 12, 2018

Conversation

alextech
Copy link
Contributor

@alextech alextech commented Aug 16, 2018

This filter is a dependency for PSR-7 input filter and form validation of file fields, and acts as a prerequisite for:

Users will filter file inputs as before, using FileNameUpload.

@alextech alextech changed the base branch from master to develop August 16, 2018 22:38
@alextech alextech changed the title Feature/psr uploadfile PSR-7 UploadedFile compatibility for RenameUpload filter Aug 16, 2018
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@alextech
Copy link
Contributor Author

alextech commented Aug 17, 2018

I am now wondering if this filter should return a new instance of UploadedFile at https://github.com/zendframework/zend-filter/pull/70/files#diff-7218516065f9379b209dac8a0b351324R205. I think this was the original reason I had Diactoros as suggestion and I remembered it now that I have a problem with filter and validator chain.

Problem with having this in a validator or filter chain is if follow up filters also want to do something with the file (eg, validate uploaded excel file has needed columns), UploadedFileInterface would already have ->move() ran on the stream, making follow up attempts to read the file throw exception. Worse, if relying on RenameUploader's name autogeneration not making names yourself There is also no indication of where the file was moved to: what $targetPath ended up being generated is pretty much lost.

However, downside there are some problems:

  • now the filter becomes in charge of creating new PSR parameters and something about it does not feel right. Unless there is a way for filter system to return PSR parameters back into handlers, there would be two UploadedFile instances flowing through the middleware: this original one with now invalid and outdated Stream, and one going through validation chain that has up-to-date path names
  • extra dependency (not as scary)

Will make another commit on this to see what you think, and because my system wont work otherwise.

File filters are a minority use case of the component, and adding a
dependency on psr/http-message and zendframework/zend-diactoros is
overkill if you are not using them. As such, I have made
psr/http-message-implementation a suggested dependency, and
zendframework/zend-diactoros a development dependency.

I have also allowed either the 1.X or 2.X series of Diactoros, as they
both fulfill PSR-7.

Finally, to allow tests to run against 5.6 and 7.0, I have added
Diactoros as a legacy dependency (so that the 1.X version can be
installed), as well as zend-crypt (but only against 5.6; so that
paragonie/random_compat can receive a version compatible with PHP 5).
src/File/RenameUpload.php Outdated Show resolved Hide resolved
src/File/RenameUpload.php Outdated Show resolved Hide resolved
@weierophinney weierophinney force-pushed the feature/psr-uploadfile branch 9 times, most recently from f90bd35 to 4469b7c Compare December 12, 2018 21:51
First, the only development requirement when testing is now
psr/http-factory (which also brings in psr/http-message). Diactoros is
no longer required.

However, PSR-17 requires PHP 7 and up. As such:

- we cannot run the integration test in PHP versions prior to 7.
- we have to remove psr/http-factory in build environments less than PHP 7.
  This must be done PRIOR to any installation attempts.
This patch extracts the logic specific to filtering a PSR-7
`UploadedFileInterface` instance to its own method,
`filterPsr7UploadedFile()`. `filter()` now detects
`UploadedFileInterface` instances early in the method, and, when found,
returns the return value of the new method. This keeps the logic within
`filter()` simpler, and ensures all logic related to filtering PSR-7
artifacts is in one place.
…name

This patch extracts two methods from the `filter()` method, `filterStringFilename()` and
`filterSapiUploadedFile()`. These allow us to check for the specific
conditions that we can accommodate, and then perform logic specific to
each scenario. This will simplify future maintenance.
Should be just `stream_factory`, not `stream_file_factory`.
Since we have discrete conditionals for each `$value` type we can
accept, we can omit the initial conditional.
@weierophinney weierophinney merged commit b5f3bde into zendframework:develop Dec 12, 2018
@Slamdunk
Copy link
Contributor

This PR introduced BC breaks for traditional SAPI, reported in #76, #77.

Fix available at #79

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

Successfully merging this pull request may close these issues.

4 participants