biig-io / melodiia Goto Github PK

Currently Melodiia doesn't use declare(strict_types=1); but it's a good thing to prepare the future to have it inside. It should be add to Melodiia before v1.0.

customDataMapper seems too generic. What about using a more significant name ?

Currently something like this is required:

        if ($form->isSubmitted() && !$form->isValid()) {
            return new FormErrorResponse($form);
        }

        if (!$form->isSubmitted()) {
            return new FormNotSubmittedResponse();
        }

The FormErrorResponse should return something also for non-submitted form.

The major issue of Melodiia at this time is his documentation completely blank.

One major improvement of Melodiia would be a Behat test suite. Feel free to contribute. It will be done some day anyway.

Currently, the FormErrorResponse does not support null and string causes. It may occur more than we think. This should be added.

The following should be enough:

if ($cause === null) {
    return [$formError->getMessage(), $propertyPath];
}

if (is_string($cause)) {
    return [$cause, $propertyPath];
}

This would be a nice feature. No real thoughts about how to implement it ATM.

Context

The CrudableModelInterface only suggests the method getId. But this method is a core concept to Melodiia. It will be used in many contexts. I think this interface makes more models "manageable by melodiia" than crudable.

Example of the problem:

class MyController
{
    public function __invoke(ItemProvider $provider)
    {
        $items = array_filter($provider->getItems(), function ($item) {
            return $item->isPublic();
        });
        return new OkContent($items);
    }
}

The result will be miss-formatted collection. The reason is that the OkContent normalizer use the array as direct result. (which makes sense since we should be able to return "single object" as a form of array)

Here are 2 potential solutions, vote on the issue if you feel concerned with ❤️ or 🎉 for the second.

First solution ❤️

Adding a new method setCollection(true) to the OkContent response.

The previous code would look like that:

$response = new OkContent($items);
$response->setCollection(true);
return $response;

Second solution 🎉

Adding a new class "collection" to Melodiia. The previous code would look like that:

return new OkContent(new Collection($items));

WDYT?

Current behavior

lead_update:
    path: /api/v1/leads/{idLead}
    controller: 'melodiia.crud.controller.update'
    methods: ['PATCH']
    defaults: # ...

This configuration fails with error message:

Could not resolve argument $id of \"melodiia.crud.controller.update()\", maybe you forgot to register the controller as a service or missed tagging it with the \"controller.service_arguments\"?

Expected behavior

Support the ability to change the name of the id.

Implementation suggestion

Add a new method in the base controller that is able to guess what is the id field (based on the assumption that the id will always start by id).

Also, add a new configuration option that allows specifying a custom id.

Current workaround

Since controllers are final, there's no workaround. They should not be final.

Not that great. But override the entire controller. It's fine. Making your own controller is not a bad practice in Melodiia context.

The FormErrorResponse class is returning different errors format between two types of form.

First case:
I've got a form resolved by an object. Then the error format is as below :

{
  "violations": {
    "registration.email": [
      "Email is required"
    ],
    "registration.plainPassword": [
      "Password is required"
    ]
  }
}

Second case:
I've got a simple form created by a class, then the error format takes a different shape.

{
   "violations":{
      "[currentPassword]":[
         "Given password don't match with user password"
      ]
   }
}

Behavior

In basic form, error properties are named as an array adversely of an object path.

Expected

Standardize all error properties as object paths.

This is about the following option: https://github.com/biig-io/Melodiia/blob/master/src/Crud/Controller/Create.php#L71 (not specified on the linked line)

The current documentation system is based on swagger-php, which is a pain. The automatic generation of the documentation is a real pain for the maintainers of Melodiia. Documentation extension process will be a pain for users...

That's why I suggest removing the automatic generation of documentation. Melodiia will just provide swagger integration (a controller that render swagger). This means adding a new configuration item for the documentation file (yaml). We're also gonna remove any documentation item from the configuration.

Biig\Melodiia\Crud\Controller\Delete

For the moment nothing is done to be able to cancel the removing of the resource, but it could be useful for some domain reasons.

Create a new type of event (PreDeleteEvent?) accepting these methods:

• cancelDelete(): void
• isDeletion(): boolean
• delete(): void

The last listener which calls cancel or delete takes the way on other previous.
If isDeletion() return false, the call to $this->dataStore->remove($data) should be avoided.

FilterInterface is only compatible with the QueryBuilder of Doctrine ORM. Which seems ok. You can implement your own DoctrineDataStore and filter stuff to use NativeQueryFilter.

The point is, should this be supported by default in Melodiia? 👍 👎

What could be done without changing everything:

• Add a FilterCollection interface non dependent to QueryBuilder of the ORM
• Remove ORM dependent part of the FilterInterface

Please see file

I don't get what is the purpose of the string $dataClass = null parameter.

The data class can be retrieved directly from the form instance.

if (!$form instanceof FormInterface) {
    return null;
}

$dataClass = $form->getConfig()->getOption('data_class');
if (MyObject::class !== $dataClass) {
   throw new InvalidArgumentException('Expected "$dataClass" to be equals to ' . MyObject::class);
}

Moreover the ApiType, which call createObject don't give a second parameter (the dataClass), so the value is always null.

This change will make the crud of Melodiia complete 🎉 .

The filters are used to filtering a collection of items.

It is common to want a filter applying only for a specific route.

Unfortunately, the method (supports(string $class)) doesn't have the request. To implement this logic you have to inject the requestStack and do the classic checks against null value.

It could be helpful and have sense to inject the current request to the support method (and only support)

This is BC IMHO, but what do you think @Nek- ?

While calling the POST endpoint without matching all constraints, the API return a 400 status code.

The documentation should describe this use case

Building forms is kinda repetitive but it is the recommended way to use Melodiia.

See what happens here, and here, and in any custom controller.

We could provide the following trait to make things smoother, here is an example of implementation:

use Symfony\Component\Form\FormFactoryInterface;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Exception\HttpException;
use Zend\Json\Exception\RuntimeException;
use Zend\Json\Json;

trait GetFormHydratedWithJsonRequest
{
    private function getForm(Request $request, FormFactoryInterface $formFactory, string $type, $data = null): FormInterface
    {
        $form = $formFactory->createNamed('', $type, $data);
        try {
            $requestContent = Json::decode((string) $request->getContent(), Json::TYPE_ARRAY);
        } catch (RuntimeException $e) {
            throw new HttpException(Response::HTTP_BAD_REQUEST, 'Invalid input data', $e);
        }
        $form->submit($requestContent, Request::METHOD_PATCH !== $request->getMethod());

        return $form;
    }
}

A common use case while dealing with collections is to add/remove/update during a single request.

I suggest a behavior to handle that

• Create item -> A JSON object (representing the item to create) without identifier should be submitted.
• Update item -> A JSON object (representing the item to update) with identifier should be submitted. If the identifier is unknown, a 404 response is returned.
• Delete item -> If an object that was present in the collection isn't submitted by the client, we assume it's a deletion.

"Identifier" could be a property path or a callback function.

What do you think?

DomainObjectsDataMapper is used as default data mapper but works only for value objects. It may be a good idea to verify if the current objet is not a value object and keep the old object in this case.

    public function mapFormsToData($forms, &$data)
    {
        if ($data instanceof CrudableModelInterface) {
            return $data;
        }
        $data = $this->createObject($forms, !empty($data) && is_object($data) ? get_class($data) : null);
        parent::mapFormsToData($forms, $data);
    }

Maybe this piece of code is a good idea. I'm not sure it's actually is but well. We need to think about.

In any case, considering the option "customDataMapper", there's no problem that the default datamapper does not support every case.

It should be the default behaviour.

PUT should be used to replace entirely.

Current situation:

If we need to change a serialization context option we need to override completely the SerializeOnKernelView.

Potential solutions

Having a method getContext() could be a start (extending is better than complete override IMHO). But the best thing to do is to provide a context builder mechanism. "ala api p".

Melodiia provides a nice data mapper that handle domain objects. This is a nice feature. The main problem to that is that this data mapper has no parameter to provide additional metadata to build an object (which is most part of the time needed).

I suggest to modify the createObject method to make it the following:

DomainObjectsDataMapper::createObject(iterable $form, string $dataClass, array $additionalData = []);

CRUD controllers do not handle JSON errors and return 500 errors. We should return 400 error.

The solution is basically to add a try { } catch (JsonException $e) { return new WrongInputData(); }.

See #64

The following code should be default for date time type in the context of an API. We should provide a correct DateTimeType:

namespace Melodiia;

class DateTimeType extends AbstractType
{
    /**
     * {@inheritdoc}
     */
    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver
            ->setDefaults([
                'widget' => 'single_text',
                'format' => DateTimeType::HTML5_FORMAT,
            ])
        ;
    }
}