Better JSON CRUD

[Skullbock]

Discussion started here: https://github.com/akeeba/fof/pull/520

Hi @nikosdion while dealing with the implementation you mentioned here https://github.com/akeeba/fof/pull/520 i found out that the onAfterDispatch part of the trait doesn't solve the issue in the case of errors.

Basically, if the $this->controller->execute() throws an exception, it should be intercepted by the trait and dealt with, but the onAfterDispatch runs only if the controller->execute() returns true.

Also, if i don't apply the redirect, the save() operation should return the saved json document, but doing so from the dispatcher (or a trait of the dispatcher) doesn't seem right to me

[nikosdion]

Ah, crap... Does it really require a b/c break in the DataController or am I missing something? Any good ideas?

[Skullbock]

it could still be done as a trait, but in the controller maybe? Also, is b/c enforce while in beta in fof3? i think i could make it so that the only thing changed would be in json views, which do not work currently so it would be a bug fix, not a b/c break i think

[nikosdion]

FOF 3.0.0 has gone stable so yeah, b/c is pretty much in effect :)

If you can implement this as a Controller trait I'm all for it. It's an optional feature so I don't mind if it doesn't work on PHP 5.3. Or if you can bake it in the DataController or Controller that'd be fine by me (pending a code review once you write it, of course).

[Skullbock]

Trying that :)

[Skullbock]

Ok, so from my research i would try to do something like this, tell me if it sounds reasonable (and of corse b/c compatible)

1. I would just work on DataController, since it's where we deal with CRUD json tasks
2. Change the way we deal with errors in the single tasks. Currently we have some try / catch blocks in there and just deal with each specific exception locally (usually just with a $this->setRedirect) and then return. Since every error is an exception, i would wrap either the execute() or each tasks method with a general try / catch block and implement a switch to deal with the various formats and way of dealing with the errors (maybe this can go into a trait?)
3. Then we identify the best way to deal with errors. Normal HTML format would stick with redirects, while json would go with header status code + json error reporting, while csv could just output a message or something.

[nikosdion]

I don't like #2. There is a reason we catch errors locally. We need to do some form of error management. We also do redirects both on success and on error depending on the task. Do not just bubble up the exception, it would be a massive b/c break that would warrant a new major release version. That's not in the cards.

Instead I think it's better to add some code in the execute() method to detect redirects when we're in a JSON view and handle error and success redirects differently.

[Skullbock]

i'll try and let you see some code. For a next refactor i would like to think error redirects differently. I see them very similarly to output buffering. They should be deal with, but probably in a more structured manner

[nikosdion]

Actually​, not just errors.

In a next major version I'd go for a document renderer which handles the entire request output. Errors and success messages could then be rendered appropriately.

[Skullbock]

Yep that's what i meant :) But i think i found the right way in current version, posting PR soon

[Skullbock]

Tell me what you think. Still dirty but i like the onAfterSave concept in the JsonView