CakeDC/Users does not play well with FriendsOfCake/Crud for APIs (using Crud.Api listener)

[mortinp]

Hi there,

First of, I'll make it clear that I know it is not mandatory that one plugin plays well with another plugin, but I guess what I will discuss here talks about CakeDC/Users agnosticity in some way.

My use case: I want to allow user registration through an API, and I'm trying to combine CakeDC/Users with FriendsOfCake/Crud with Crud.Api listener.

What I did:

1. I loaded Crud plugin along with the Crud.Api listener.
2. I created a separate Controller and I use CakeDC\Users\Controller\Traits\RegisterTrait inside it to be able to "adopt" the register() function.
3. All routes and configuration well setup: it does execute the code I want it to execute in the server.

PROBLEM #1 (not related to FriendsOfCake/Crud):

If I send a POST request through the API with validation errors attempting to register a user, the RegisterTrait will try to use the Flash component to display the error when the registration fails.

Problems I see:

1. The RegisterTrait forces me to load the Flash component in my controller.
2. The RegisterTrait disregards the request's headers Accept and Content-Type, and assumes the response will be html-formatted.
3. The RegisterTrait is too coupled to the Flash component, which it shoudn't.

PROBLEM #2:

The RegisterTrait never throws an error and doesn't dispatch an "OnError" event. Because of that, the Crud API never creates an appropriate view error in the response.

What the RegisterTrait does is it just flashes the error message and sets the user data.

So in the end, this is the json response I get from the API:

{
    "user": {
        "role": "user",
        "validated": false,
        "active": true,
        "activation_date": "2020-11-16T22:29:40+00:00"
    }
}

Which is just the user variable set in the viewVars at RegisterTrait: line 87.

THINGS TO THINK ABOUT

I would look into making RegisterTrait less coupled to other dependencies. In my specific case it could be done in a few ways:

1. Look into the request's headers to see the type of request and act accordingly.
2. Throw an Exception instead of flashing the error message.
3. Don't throw or flash the message, but dispatch an "AfterRegisterFailed" event.

CONCLUSION

These "problems" indicate a few handicaps from the plugin, from my point of view. I'm not sure if this is just an isolated case or maybe it's something other people are thrying to do and might have come across.

I also haven't looked into this deeply, so I might be very wrong in my assumptions.

What do you all think?

[steinkel]

Hi @MProenza and thank you for the suggestions.

I feel you're trying to use RegisterTrait to do more than initially intended, the coupling with the FlashComponent makes sense as the original intent of the trait was to be used in the scope of your controller. One way to handle the situation could be refactoring the trait a bit to make it easier for users to extend it and get rid of flash dependencies. Another way to fix the situation would be handling the flash call from your controller to do nothing, but it's not as clean as the first solution. I don't see a clear way to fix this let me take a deeper look and think about it, thanks!

[steinkel]

Regarding integration with crud api, the solution here would be extending the trait as the integration with crud would require other modifications, the initial scope of the plugin was working with a plain cake application baked from scratch.

[mortinp]

Hi @steinkel , thanks a lot for your reply and sorry for replying so late :)

I already did some "not so clean" adaptations in my own code and it is working well now. I ended up using your own CakeDC/cakephp-api in the end! It required some adpatations any way.

Anyway, the coupling to Flash component is something definitely worth solving, I think. I still keep with my dirty adaptations to make it work ;)

I think a good way would be to allow the configuration of some "ErrorHandler" to handle all the plugin's errors. The plugin would come with a "FlashErrorHandler" by default which just flashes the error (which is what the plugin already does), so every code base using your plugin already wouldn't break, but it would be extendable.

How I imagine this would work?

1. In every call to $this->Flash->error(), I would call $this->errorHandler->handle($this, $errorCode, $errorMessage). I would pass $this, which is the "caller" because maybe the client code wants to use it to do something (ie. $caller->Flash)
2. I would provide a "FlashErrorHandler by default" which implements the method handle(), like:

public function handle($caller, $errorCode, $errorMessage) {
   $caller->Flash->error($errorMessage);
}

3. Up to this point, all client code remains working like before, but...
4. I would provide a way to set a different "ErrorHandler" via plugin configuration, like:

[
    "Users" => ["Error" => [
        "errorHandler" => MyErrorHandler::class
    ]]
]

Now the plugin is extendable and I would be able to implement a different ErrorHandler with the logic I prefer.

Of course the details are up to consideration, but I guess this would work okay.

Thanks for your help!

[ajibarra]

@steinkel is this still valid? is there anything needed here?