V8-archive: item.create and item.update hooks not executed

[jbitsch]

Bug Report

Steps to Reproduce

1. git clone https://github.com/directus/api .
2. composer install
3. Copy the old version of the configuration file(api.php) to config/

4. Create test item create hook, either as item.create or item.create.TABLE_NAME
   ````
   'actions' => [
   'item.create' => function (array $data) {
   $client = new \GuzzleHttp\Client([
   'base_uri' => 'https://example.com'
   ]);

   $data = [
   'type' => 'post',
   'data' => $data,
   ];

   $response = $client->request('POST', 'alert', [
   'json' => $data
   ]);
   }
   ]
   ````

Expected Behavior

The endpoint should be called

Actual Behavior

The code is never executed and will therefore not call the endpoint.

Other Context & Screenshots

Technical Details

• Device: Desktop
• OS: MacOS 10.14.5
• Web Server: Apache 2.4.37
• PHP Version: 7.2.0
• Database: MySQL 8.0.12
• Install Method: cloned master branch

[jbitsch]

A note to this. We used a very old version before, which was from 28th of January, and here it works perfect. So something in between then and now have broken it.

[bjgajjar]

@WoLfulus

It seems like again its a configuration issue. I guess #1096 resolved the configuration issue of storage only. I tried to replicate the issue and able to find out that the hooks define in the api.php is not able to get in register_global_hooks function.

So the hooks which are defined in the api.php are not registered and that's why not being executed.

Can you please check it once at your end? Let me know if you want any help to replicate this issue.

[Lapsus]

I have the same problem here. After upgrading to 2.3.0 the hooks in my api.php are no longer triggered.

[bjgajjar]

Hey @Lapsus

@WoLfulus is working on that. It may be resolved in the next release. :)

[Lapsus]

I am sorry if i barge in here.

But ...

1. current state is, the configuration from api.php is not passed correctly. This seems like a major issue to me. I am willing to help to get a quick fix here.
2. instead of passing the config directly (as before) the config now gets validated by a schema definition. This is where the hooks data from api.php gets lost as far as i was able to find out.
3. this validation of the configuration runs on every single request and walks recursive thru the configuration. So the more configuration options you have the longer this takes (even if it is blazing fast it is still slow compared to "not done at all".). It also adds defaults for missing configuration options (!!!).

So is this validation really necessary?
Why is it necessary?
Why not stay on the simple version we had before. And just require the configuration.
Getting the configuration from ENV instead of api.php should be possible "the old way" as well.

Valdiating and adding reasonable defaults for the config may seem useful. But in my opinion is not, it is dangerous. Because if you miss configure your system (say forget database config) you no longer get an error if Directus can connect to "root@localhost" with password "root" and you have a dabatase named directus. Everything seems to work, but it actually does not.

But this are just my thoughts here. I would not validate the config and add defaults. If the config is incorrect, faulty or things are missing. The system should stop working. And not having to parse/validate it on every request helps performance as well.

[benhaynes]

Thanks @Lapsus — any thoughts on these points @WoLfulus?

[WoLfulus]

Hi! Sorry about that!

1. current state is, the configuration from api.php is not passed correctly. This seems like a major issue to me. I am willing to help to get a quick fix here.

Yeah, there are some bugs popping after we changed the configs, and I'd love some help with that.

2. instead of passing the config directly (as before) the config now gets validated by a schema definition. This is where the hooks data from api.php gets lost as far as i was able to find out.

Binal already tracked the issue for about a week ago, but I haven't had enough time to sit and fix that issue. @bjgajjar would you mind commenting your findings?

3. this validation of the configuration runs on every single request and walks recursive thru the configuration. So the more configuration options you have the longer this takes (even if it is blazing fast it is still slow compared to "not done at all".). It also adds defaults for missing configuration options (!!!).

Why is it necessary?

This change was due to docker support for loading environment variables. All environment values comes as strings and we must convert them to proper values. The whole schema stuff was introduced to support both environment variables and api.php config files and yet avoid introducing a breaking change from how configs are loaded.

Why not stay on the simple version we had before. And just require the configuration.

We can bypass the config schema if loading from file or implement some caching strategy. I'd prefer the second option since we branch the behavior if we bypass the schema for other config data sources (for example loading from a redis/consul server - which is a real use case and another reason why I added the config sources/schema stuff)

Getting the configuration from ENV instead of api.php should be possible "the old way" as well.

As mentioned before, it's not just files and environment variables we're covering here even though these are the ones currently implemented.

Valdiating and adding reasonable defaults for the config may seem useful. But in my opinion is not, it is dangerous. Because if you miss configure your system (say forget database config) you no longer get an error if Directus can connect to "root@localhost" with password "root" and you have a dabatase named directus. Everything seems to work, but it actually does not.

The defaults were introduced for minimal setup of values. The default values are taken from the example api_sample.php file for example. We're also thinking about validating the installation through direct API tests even though I don't know the state of this (@benhaynes might be able to comment on that)

But this are just my thoughts here. I would not validate the config and add defaults. If the config is incorrect, faulty or things are missing. The system should stop working. And not having to parse/validate it on every request helps performance as well.

I was concerned about that too and thats why I've added some benchmark scripts to the repository, but I haven't seen enough impact with the changes (schema helps with that as it knows every possible value and won't just start searching/matching patterns with underscores and stuff in a more naive approach)

IMHO there are a lot of changes that I personally would like to see in setup/installation/extensibility side of directus, but that would mean a new major release with a lot of breaking changes, and we just started releasing Directus 7.

We're always open to different thoughts and approaches too, we just have to keep in mind that docker support must exist as there are more and more requests for that (and it should actually make sense for people experienced with docker), also there shouldn't be any breaking changes that needs manual fixing when upgrading directus (current issues are implementation bugs and not actual breaking changes)

[Lapsus]

@WoLfulus Thanks for you detailed reply.

Just a few follow up questions. Did you consider for example using a third party library like https://github.com/vlucas/phpdotenv to get the values from environment? And therefor skipping/simplifying the schema valdiation?

[bjgajjar]

would you mind commenting your findings?

@WoLfulus Sure. :)

Hey @Lapsus,

Currently, Schema returning the default value if your config is Array. That's why you are getting the default hooks (It's an array) instead of that one which you have defined.

[Lapsus]

@WoLfulus @bjgajjar @benhaynes

Please check the PR. I fixed the default value problem for array types in schema. Also reduced some duplicate code and added a constant for the array type as is defined for strings, integer, float and boolean types.