Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

Add response action #700

Merged
merged 2 commits into from
Jan 16, 2019
Merged

Add response action #700

merged 2 commits into from
Jan 16, 2019

Conversation

rijkvanzanten
Copy link
Member

This PR adds a new action called response. It will fire whenever the API is responding to the client.

Why

The existing filters hook response allows you to change the JSON output for successful response, however, it doesn't give you any information on the request/response and it doesn't fire on unsuccessful requests, like 405s or 400s. Also, filters are meant to be used to modify the JSON output, while actions are meant to be used for other purposes. This action can be used to integrate some sort of logging service via web hooks into the API.

Usage

'hooks' => [
  'actions' => [
    'response' => function (array $responseInfo, array $data) {
      // Response info will contain path, query, status, method, and size
      // $data is the actual data that is about to be returned to the client
    }
  ]
]

Notes

For some reason, $this->emitter is null for the MethodNotAllowedHandler. I wasn't able to figure out why NotFoundHandler has a emitter, but MethodNotAllowedHandler doesn't. Maybe you can shed some light on that @wellingguzman?

I'm not too happy about the fact that the triggerResponseAction is duplicated in 2 files. I wasn't sure if you like it to be a separate file or not @wellingguzman.

Copy link
Contributor

@wellingguzman wellingguzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, $this->emitter is null for the MethodNotAllowedHandler. I wasn't able to figure out why NotFoundHandler has a emitter, but MethodNotAllowedHandler doesn't. Maybe you can shed some light on that @wellingguzman?

You can change this and this line to pass the HookEmitter ($container['hook_emitter']), and because both classes will be extending from ErrorHandler, the constructor will be expecting the emitter as first parameter.

From:

// src/core/Directus/Application/DefaultServicesProvider.php:43

if (!isset($container['notFoundHandler'])) {
  $container['notFoundHandler'] = function () {
    return new NotFoundHandler();
  };
}

if (!isset($container['notAllowedHandler'])) {
  $container['notAllowedHandler'] = function () {
    return new MethodNotAllowedHandler();
  };
}

To:

// src/core/Directus/Application/DefaultServicesProvider.php:43

if (!isset($container['notFoundHandler'])) {
  $container['notFoundHandler'] = function () use ($container) {
    return new NotFoundHandler($container['hook_emitter']);
  };
}

if (!isset($container['notAllowedHandler'])) {
  $container['notAllowedHandler'] = function () use ($container) {
    return new MethodNotAllowedHandler($container['hook_emitter']);
  };
}

I'm not too happy about the fact that the triggerResponseAction is duplicated in 2 files. I wasn't sure if you like it to be a separate file or not @wellingguzman.

It's okay with can either create a helper or a trait or leave it as is for now, this probably won't change for while.

@rijkvanzanten
Copy link
Member Author

Interestingly enough, the NotFoundHandler already had access to the emitter somehow 🤔

I'll change those lines 👍

@wellingguzman wellingguzman added this to To do in v2.0.15 Jan 16, 2019
@wellingguzman
Copy link
Contributor

@rijkvanzanten is this PR ready to be merged?

@rijkvanzanten
Copy link
Member Author

It works as advertised on my end, so if you're happy with the code it's good to go 😄

@wellingguzman
Copy link
Contributor

Great! This looks good to me.

@wellingguzman wellingguzman merged commit 18b9ead into master Jan 16, 2019
@rijkvanzanten rijkvanzanten deleted the response-action branch January 16, 2019 15:32
@wellingguzman wellingguzman moved this from To do to Done in v2.0.15 Jan 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
v2.0.15
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants