laminas-api-tools/api-tools-content-validation

Laminas module providing incoming content validation

api-tools-content-validation's Issues

PHP 8.0 support

Feature Request

Q	A
New Feature	yes

Summary

To be prepared for the december release of PHP 8.0, this repository has some additional TODOs to be tested against the new major version.

In order to make this repository compatible, one has to follow these steps:

• Modify composer.json to provide support for PHP 8.0 by adding the constraint ~8.0.0
• Modify composer.json to drop support for PHP less than 7.3
• Modify composer.json to implement phpunit 9.3 which supports PHP 7.3+
• Modify .travis.yml to ignore platform requirements when installing composer dependencies (simply add --ignore-platform-reqs to COMPOSER_ARGS env variable)
• Modify .travis.yml to add PHP 8.0 to the matrix (NOTE: Do not allow failures as PHP 8.0 has a feature freeze since 2020-08-04!)
• Modify source code in case there are incompatibilities with PHP 8.0

Suggestion for improvement regarding entity/collection handling in next major release

I noticed this recent change #96 where specific filter configuration for collections is introduced:
I missed this change, otherwise I would have commented sooner, but wouldn't it have been nicer to make the configuration array like so:

'zf-content-validation' => [
    'Application\Controller\HelloWorld' => [
        'input_filter' => 'Application\Controller\HelloWorld\Validator',
        'collection' => [
            'PUT' => 'Application\Controller\HelloWorld\UpdateValidator',
        ],
        'entity' => [
            'POST' => 'Application\Controller\HelloWorld\CreateValidator',
        ]
    ],
],

Like that the default input filter could be set for all requests (if available) and then for each case either entity or collection it can be collected and overwritten.
It would be more readable IMO instead of introducing all these additional METHOD_COLLECTION keys in the configuration.

I understand this is now too late, but might be worth reconsidering reorganizing keys in Apigilty next major release 2.0. I noticed more collection and entity specific keys are being added to the application while they do the same thing:

METHOD/METHOD_COLLECTION
collection_http_methods/entity_http_methods
collection_class/entity_class

some are specific for collection only
collection_name, collection_query_whitelist

They could also be nested in a key entity, or collection and then there could an interface for the common/shared elements but specific class instance is used depending on whether we are handling a entity or collection request.

This entity/collection key organization would be in line with the configuration of for example MvcAuth:

'authorization' => [
    'Controller\Service\Name' => [
        'actions' => [
            'action' => [
                'default' => boolean,
                'GET' => boolean,
                'POST' => boolean,
                // etc.
            ],
        ],
        'collection' => [
            'default' => boolean,
            'GET' => boolean,
            'POST' => boolean,
            // etc.
        ],
        'entity' => [
            'default' => boolean,
            'GET' => boolean,
            'POST' => boolean,
            // etc.
        ],
    ],
],

Then even the metadata_map could be reorganized the same way. Now the difference is made using a 'is_collection' key set to true/false, but even there there the entries could be grouped by collection or entity:

'metadata_map' => [
    'entity' => [
    ],
    'collection' => [
    ]
]

The MetadataMap has those organized in two groups, metadata for entities and collections.
Those could even be stored in two different Metadata classes, one for collections and one for entities.

Those metadata objects for collections and entities are not the same, only a part of their interface is common. For example EntityMetadata holds entity specific properties and the hydrators for the entity,
CollectionMetadata holds the collection specific data

This differentiation between work done separately for collections and entities currently works all the way down into the HalJsonRenderer logic:

    public function render($nameOrModel, $values = null)
    {
        if (! $nameOrModel instanceof HalJsonModel) {
            return parent::render($nameOrModel, $values);
        }

        if ($nameOrModel->isEntity()) {
            $helper  = $this->helpers->get('Hal');
            $payload = $helper->renderEntity($nameOrModel->getPayload());
            return parent::render($payload);
        }

        if ($nameOrModel->isCollection()) {
            $helper  = $this->helpers->get('Hal');
            $payload = $helper->renderCollection($nameOrModel->getPayload());

            if ($payload instanceof ApiProblem) {
                return $this->renderApiProblem($payload);
            }
            return parent::render($payload);
        }
    }

Should it not be:

public function render($nameOrModel, $values = null)
{
    if (! $nameOrModel instanceof HalJsonModel) {
        return parent::render($nameOrModel, $values);
    }
    $helper  = $this->helpers->get('Hal');
    $payload = $helper->render($nameOrModel);
    return parent::render($payload);
}

Getting the payload can be handled inside the render method.
Or am I maybe missing something here?

Please don't see this as criticism, I love the work everyone does in the zf-campus repositories. Merely see this as suggestion for possible future improvement. I am not a great software architect myself, but just wanted to share my thoughts and hope it will lead to some food for thought and/or discussion.

---

Originally posted by @Wilt at zfcampus/zf-content-validation#100

[RFC]: Allow custom CollectionInputFilter in ContentValidationListener

RFC

Q	A
Proposed Version(s)	1.9.1
BC Break?	No

Goal

In my application I want validate the whole collection. For example one of items must have "id" property with value "0".
So I'm going to extend Laminas\InputFilter\CollectionInputFilter and override isValid() with own logic after data is filtered and validated by internal InputFilter.

Background

The problem is inside Laminas\ApiTools\ContentValidation\ContentValidationListener line 222

if ($isCollection && ! in_array($method, $this->methodsWithoutBodies)) {
            $collectionInputFilter = new CollectionInputFilter();
            $collectionInputFilter->setInputFilter($inputFilter);
            $inputFilter = $collectionInputFilter;
}

It replaces my own CollectionInputFilter.

Considerations

Proposal(s)

My proposal is slightly modify the condition

if ($isCollection && ! in_array($method, $this->methodsWithoutBodies) && ! $inputFilter instanceof CollectionInputFilter)

Then it would be possible to use custom CollectionInputFilter, the internal InputFilter can be initialized by custom Factory or by Laminas\InputFilter\Factory after it gets injected into CollectionInputFilter by initializer.
The users can set own CollectionInputFilter using '<METHOD>_COLLECTION' setting.

Appendix

Psalm integration

Feature Request

Q	A
QA	yes

Summary

As decided during the Technical-Steering-Committee Meeting on August 3rd, 2020, Laminas wants to implement vimeo/psalm in all packages.

Implementing psalm is quite easy.

Required

• Create a psalm.xml in the project root
• Copy and paste the contents from this psalm.xml.dist
• Run $ composer require --dev vimeo/psalm
• Run $ vendor/bin/psalm --set-baseline=psalm-baseline.xml
• Add a composer script static-analysis with the command psalm --shepherd --stats
• Add a new line to script: in .travis.yml: - if [[ $TEST_COVERAGE == 'true' ]]; then composer static-analysis ; fi
• Remove phpstan from the project (phpstan.neon.dist, .travis.yml entry, composer.json require-dev and scripts)

Optional

• Fix as many psalm errors as possible.

Problem with required field, when type is set

Hello!

Found such a case - if field is not set to required, but value is set to false:

[ 'required' => false, 'validators' => [], 'filters' => [], 'name' => 'debug', 'field_type' => 'boolean', ]

On request like this:

{"debug":false}

It will give an error:

{"validation_messages":{"debug":{"isEmpty":"Value is required and can't be empty"},"type":"http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html","title":"Unprocessable Entity","status":422,"detail":"Failed Validation"}

---

Originally posted by @tasselchof at zfcampus/zf-content-validation#90

Filters throw uncaught exceptions and return improper response code

When validating the supplied data it's possible for a Filter that is attached to the InputFilter to throw an Exception. If this happens, Apigility will return a status code of 200 and depending on your settings it may show the Exception message or stack trace.

I propose wrapping the

$inputFilter->isValid()

call on https://github.com/zfcampus/zf-content-validation/blob/master/src/ContentValidationListener.php#L216 in a try/catch block, and returning a ApiProblemResponse with a status code of 422 should an Exception be thrown.

---

Originally posted by @ashireman at zfcampus/zf-content-validation#32

Validator with Zend\I18n\Validator\DateTime is always required

I am attempting to use the DateTime validator in an Apigility project. The configuration is set to not be required and continue if empty, however the validator always returns a validation error.

Input

{"name": "Joe Public", "email": "[email protected]"}

Validator configuration

1 => array(
    'name' => 'dateField',
    'required' => false,
    'filters' => array(),
    'validators' => array(
        0 => array(
            'name' => 'Zend\\I18n\\Validator\\DateTime',
            'options' => array(
                'pattern' => 'Y-m-d\TH:iP',
                'message' => 'Date format must be Y-m-d\TH:iP',
            ),
        ),
    ),
    'allow_empty' => true,
    'continue_if_empty' => true,
    'description' => 'Date field',
),

Error message

{
  "validation_messages": {
    "dateOfBirth": {
      "datetimeInvalid": "Date format must be Y-m-d\TH:iP"
    }
  },
  "type": "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html",
  "title": "Unprocessable Entity",
  "status": 422,
  "detail": "Failed Validation"
}

---

Originally posted by @alexisbmills at zfcampus/zf-content-validation#49

Some non corresponding between 'zf-content-validation' and 'zf-rest'

There is a some non corresponding between 'zf-content-validation' and 'zf-rest'.
In PUT request when sent router identifier as query parameter zf-content-validation understood as entity because in ContentValidationListener.php isCollection() there is also check return (null === $request->getQuery($identifierName, null)); on line 547.

/**
     * Does the request represent a collection?
     *
     * @param string $serviceName
     * @param array $data
     * @param RouteMatch|V2RouteMatch $matches
     * @param HttpRequest $request
     * @return bool
     */
    protected function isCollection($serviceName, $data, $matches, HttpRequest $request)
    {
        if (! array_key_exists($serviceName, $this->restControllers)) {
            return false;
        }
        if ($request->isPost() && (empty($data) || ArrayUtils::isHashTable($data))) {
            return false;
        }
        $identifierName = $this->restControllers[$serviceName];
        if ($matches->getParam($identifierName) !== null) {
            return false;
        }
        return (null === $request->getQuery($identifierName, null));
    }

and run validation for entity, but in Resource called collectin method replaceList() and not update(), because in RestController.php getIdentifier() check router identifier only in param and not query.

/**
     * Retrieve the identifier, if any
     *
     * Attempts to see if an identifier was passed in the URI,
     * returning it if found. Otherwise, returns a boolean false.
     *
     * @param  \Zend\Mvc\Router\RouteMatch $routeMatch
     * @param  \Zend\Http\Request $request
     * @return false|mixed
     */
    protected function getIdentifier($routeMatch, $request)
    {
        $identifier = $this->getIdentifierName();
        $id = $routeMatch->getParam($identifier, false);
        if ($id !== null) {
            return $id;
        }
        return false;
    }

Same non corresponding also in GET, DELETE and PATCH requests.

---

Originally posted by @ruzann at zfcampus/zf-content-validation#107

Unexpected behavior with "use_raw_data"

If I set my controller's option for use_raw_data to false, I'd expect that instead of getting everything that's passed in (unfiltered expected fields as well as all the other stuff someone may be passing in) I would only get the filtered, expected fields. Previously it was common to grab just the filtered and expected values in the resource with something like

$data = $this->getInputFilter()->getValues()

However, it appears that if use_raw_data is false, you'll get the filtered expected values plus any other unfiltered, unexpected values as well which seems to be less than ideal.

Previously it was possible to ignore any incoming parameters that you didn't care about and the API would work. There is another new setting "allows_only_fields_in_filter" which if set to true means that it will actually send back a 422 validation error response for the fields that are not defined in your input filter.

It seems it would make sense to allow the resource method to receive only the valid, filtered values that were expected and throw away the rest. Right now the only way to do that is to grab the input filter from within the method and call getInputFilter()->getValues() or something similar.

I'd appreciate any thoughts or comments on it. Perhaps I'm just doing it wrong. It does seem that it'd be simpler to test if I knew I could just pass in and test with validated and filtered values and not need to worry about the input filter dependency within the method.

Thank you.

---

Originally posted by @dstockto at zfcampus/zf-content-validation#59

DELETE method can not have an input_filter

Because of ContentValidationListener.php:63 it is not possible to validate the body of a DELETE request (contrary to the README.md).

IMHO the implementation is wrong, DELETEcan have a body according to RFC 7231, so DELETE should be removed from ContentValidationListener::methodsWithoutBodies.

---

Originally posted by @BreiteSeite at zfcampus/zf-content-validation#60

PATCH request to collection endpoint with empty body causes RuntimeException

When sending a PATCH request to a collection endpoint the following things happen within the ContentValidationListener's onRoute:

$data = in_array($method, $this->methodsWithoutBodies)
    ? $dataContainer->getQueryParams()
    : $dataContainer->getBodyParams();
if (null === $data || '' === $data) {
    $data = [];
}

For a PATCH request with an empty content body, this will result in $data = []

if ($isCollection && ! in_array($method, $this->methodsWithoutBodies)) {
    $collectionInputFilter = new CollectionInputFilter();
    $collectionInputFilter->setInputFilter($inputFilter);
    $inputFilter = $collectionInputFilter;
}

$isCollection is true since we are hitting a collection endpoint and PATCH is not a method without a body, so a collection input filter is created and set to the input filter for this request.

Then the input filter is validated against, for a PATCH request the $this->validatePatch() function is fired:

$status = ($request->isPatch())
    ? $this->validatePatch($inputFilter, $data, $isCollection)
    : $inputFilter->isValid();

Which then will return $inputFilter->isValid(), which will always be a CollectionInputFilter at this point.

This is the beginning CollectionInputFilter's isValid()

public function isValid($context = null)
{
	$this->collectionMessages = [];
	$inputFilter = $this->getInputFilter();
	$valid = true;

	if ($this->getCount() < 1) {
	    if ($this->isRequired) {
	        $valid = false;
	    }
	}

$this->getCount() performs a count() on $this->data and evaluates 0:

public function getCount()
{
    if (null === $this->count) {
        return count($this->data);
    }
    return $this->count;
}

But $this->isRequired is false, unless there is a listener set up for ContentValidationListener::EVENT_BEFORE_VALIDATE which changes the inputFilter's required setting. So without a listener set up, $valid = true at this point.

Here's the next few lines of isValid():

if (count($this->data) < $this->getCount()) {
    $valid = false;
}

if (! $this->data) {
    $this->clearValues();
    $this->clearRawValues();

    return $valid;
}

At this point, $this->getCount() will return the same value as count($this->data) and therefore the conditional will fail and $valid = true.

$this->data is currently [], which is a "falsey" value and will cause the conditional to be true. The method then calls two functions and returns true because $valid = true.

We then return to execution of onRoute() via ContentValidationListener:

if ($status instanceof ApiProblemResponse) {
    return $status;
}
// Invalid? Return a 422 response.
if (false === $status) {
    return new ApiProblemResponse(
        new ApiProblem(422, 'Failed Validation', null, null, [
            'validation_messages' => $inputFilter->getMessages(),
        ])
    );
}

$status is currently true as returned by $this->validatePatch(), so neither of these conditionals is met.

We then get to this part of onRoute:

if (! $inputFilter instanceof UnknownInputsCapableInterface
    || ! $inputFilter->hasUnknown()
) {
    $dataContainer->setBodyParams($data);
    return;
}

CollectionInputFilter is an instanceof UnknownInputsCapableInterface, so the conditional does not short-ciruit and continues with $inputFilter->hasUnknown()

CollectionInputFilter does not define it's own hasUnknown() method and instead inherits it from BaseInputFilter:

public function hasUnknown()
{
    return $this->getUnknown() ? true : false;
}

CollectionInputFilter does define a method for getUnknown() and here's the very beginning:

public function getUnknown()
{
    if (! $this->data) {
        throw new Exception\RuntimeException(sprintf(
            '%s: no data present!',
            __METHOD__
        ));
    }

Since $this->data is currently [], this conditional evaluates to true and a RuntimeException is thrown. Currently the only way of avoiding this RuntimeException is by hooking into ContentValidationListener::EVENT_BEFORE_VALIDATE and validating that the request body is not empty manually.

---

Originally posted by @aeux at zfcampus/zf-content-validation#103

Switch to GHA CI workflow

Switch from Travis-CI to GHA CI workflow, as documented at https://gist.github.com/weierophinney/b003e50c3c2667d08076caf31ebd36a4