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

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

Open
weierophinney opened this issue Dec 31, 2019 · 1 comment

Comments

@weierophinney
Copy link
Contributor

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

@Lokilein
Copy link

I guess this would also make it easier to fix laminas-api-tools/api-tools-documentation#31
As far as I can tell by a quick look at this, all "HTTP methods" given are also shown in the UI, which means, if I would add "GET_COLLECTION", I would have such an endpoint in the documentation as well.

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

No branches or pull requests

2 participants