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

Add file input compatibility with PSR-7 #172

Merged
merged 10 commits into from
Dec 17, 2018

Conversation

alextech
Copy link
Contributor

@alextech alextech commented Aug 16, 2018

Fixes #145 - PSR-7 compatibility for validating and filtering forms with inputs.

This patch allows validating forms submitted via a PSR-7 payload, and, specifically file uploads.

Prerequisite PRs:

I could not separate PSR vs HTTP $_FilES into completely separate subclasses because current implementation is stateful, so cannot simply pass calls from current FileInput to appropriate implementation - private protected properties that hold values and options get lost. FileInput is hardcoded in other repositories, eg Zend\Form as a input specification so has to remain main entry point for forms to be used in both MVC and Expressive. Consequently, did decorator-ish style approach.

@alextech alextech changed the base branch from master to develop August 16, 2018 22:45
@alextech
Copy link
Contributor Author

Looks like Travis does not like to pull from other repositories not having its own tokens.
Can rerun the job after the 2 dependencies get accepted.

@samsonasik
Copy link
Contributor

@alextech you can insert "no-api": true config inside each "repositories" config record in composer.json as follow:

    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/alextech/zend-validator",
            "no-api": true
        },
        {
            "type": "vcs",
            "url": "https://github.com/alextech/zend-filter",
            "no-api": true
        }
    ],

alextech and others added 7 commits December 17, 2018 11:04
…e instead of $_FILES.

Using own repository for zend-validator for tests to pass, as filter has tight dependency on validator.
…ate a bridge to appropriate PSR or HTTP filter implementation. Change to decorator style and move detection of value type to public facing FileInput.
- PSR-7 becomes optional; only required if validating PSR-7
  `UploadedFileInterface` instances.
- Diactoros is no longer required for testing; we can mock PSR-7
  interface.
weierophinney and others added 3 commits December 17, 2018 15:20
This patch moves the new file input decorators to the namespace
`Zend\InputFilter\FileInput`, and performs a number of minor refactors
on them to improve readability.

Tests are refactored slightly to use generators where appropriate, and
to mock PSR-7 interfaces instead of using Diactoros.
Adds a section to the file input chapter covering PSR-7, and showing how
to get the merged form and uploaded file data to pass to the input
filter.
@weierophinney
Copy link
Member

@alextech I've rebased off of current develop, and done the following:

  • Updated the dependencies for zend-filter and zend-validator to point to stable releases that have PSR-7 support.
  • Removed the zend-diactoros dependency, in favor of mocking PSR-7 interfaces in tests.
  • Pushed the new interface and implementations into a subnamespace (Zend\InputFilter\FileInput).
  • Done some minor fixes based on the final versions of zend-filter and zend-validator PSR-7 support.
  • Done some minor refactors to the FileInput (extracted a method for determining the FileInputDecoratorInterface implementation to use).
  • Added documentation for PSR-7 support in the FileInput chapter.

I think it should be ready to 🚢 now.

@weierophinney weierophinney merged commit 3b32a72 into zendframework:develop Dec 17, 2018
*/
protected function injectUploadValidator()
Copy link
Contributor

@svycka svycka Dec 18, 2018

Choose a reason for hiding this comment

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

removing protected methods isn't BC break?

Copy link
Member

Choose a reason for hiding this comment

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

This method is protected ;-) Only changes to public interface are considered as BC Break.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I mean protected :) and I thought that if you set method protected it's like saying that you are safe to extend it if you need and we will not change it.

@alextech
Copy link
Contributor Author

alextech commented Dec 18, 2018 via email

@michalbundyra
Copy link
Member

@alextech As far as I know (if I am wrong please someone correct me) - in ZF rules for public interface are "public" methods and "protected" only when these are described as can be used in extension (for example in the documentation). I don't remember where I read about it, I can't find it now :(

@weierophinney
Copy link
Member

@svycka @alextech and @webimpress

Our policy since the ZF1 days has been to treat only public as part of the public API for BC purposes. We tend to scrutinize removals of protected methods a little closer, but, as @webimpress notes, unless it was documented as part of an internal API that users can rely on (e.g., how symfony/console documents configure and execute), we don't treat changes as BC issues.

In this particular case, there's little reason for extension, and the new changes make creating new strategies for file uploads easier.

@svycka
Copy link
Contributor

svycka commented Dec 18, 2018

@weierophinney I am ok with this change just maybe this should be noted in changelog.
And good to know that protected can be removed or changed any time, will be more careful using them with ZF components

@Slamdunk
Copy link
Contributor

Like zendframework/zend-filter#70 this PR introduced a BC Break. Hope to isolate the issue and post a PR soon.

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.

7 participants