[advancedcontentfilter] Fix Hook-call during install #922

Merged

nupplaphil merged 3 commits from bug/7989-advancedcontfilter into 2019.12-rc

2019-12-23 01:38:01 +01:00

nupplaphil commented

2019-12-22 19:39:10 +01:00

(Migrated from github.com)

Fixes https://github.com/friendica/friendica/issues/7989

Types of hook-calls:

Hook::register() just registers hooks, but doesn't add them to the current call
Hook::add() just adds hook to the current call, but doesn't register them

Since we want to call the db-definition hook just once during the install process, I switched from Hook::register() to Hook:add() and therefore removed the unregister call as well.

Fixes https://github.com/friendica/friendica/issues/7989 Types of hook-calls: * `Hook::register()` just registers hooks, but doesn't add them to the current call * `Hook::add()` just adds hook to the current call, but doesn't register them Since we want to call the db-definition hook just **once** during the install process, I switched from `Hook::register()` to `Hook:add()` and therefore removed the unregister call as well.

❤️ 1

annando (Migrated from github.com) reviewed 2019-12-22 20:16:31 +01:00

MrPetovan commented

2019-12-22 20:43:08 +01:00

(Migrated from github.com)

I think this change is wrong. If the hook isn’t registered, the next time the DB update routine is called, the addon table won’t be added to the definition.

I’d rather the issue be fixed by calling Hook::add to Hook::register instead.

I think this change is wrong. If the hook isn’t registered, the next time the DB update routine is called, the addon table won’t be added to the definition. I’d rather the issue be fixed by calling `Hook::add` to `Hook::register` instead.

nupplaphil commented

2019-12-22 20:53:48 +01:00

(Migrated from github.com)

I think tis change is wrong. If the hook isn’t registered, the next time the DB update routine is called, the addon table won’t be added to the definition.

But why do we need it here? The DB-upate routine is called directly here:
838c28f171/advancedcontentfilter/advancedcontentfilter.php (L63)
And we don't need it at other places, so why should we store this hook?

I’d rather the issue be fixed by calling Hook::add to Hook::register instead.

This was my first thought, but than I found this description:
cc64471e4c/src/Core/Hook.php (L67)

My interpretation is that it is intentionally not added here. And I don't know what side-effects could cause it to other hooks. So I thought better stay at one point where only one addon could get broken instead of brick the whole hook-mechanism ;-)

> I think tis change is wrong. If the hook isn’t registered, the next time the DB update routine is called, the addon table won’t be added to the definition. > But why do we need it here? The DB-upate routine is called directly here: https://github.com/friendica/friendica-addons/blob/838c28f1713633c1ab499aabe7830d6c6259e58f/advancedcontentfilter/advancedcontentfilter.php#L63 And we don't need it at other places, so why should we store this hook? > I’d rather the issue be fixed by calling `Hook::add` to `Hook::register` instead. This was my first thought, but than I found this description: https://github.com/friendica/friendica/blob/cc64471e4c4cbda6f79c50ca784d0b927a45a6a5/src/Core/Hook.php#L67 My interpretation is that it is intentionally not added here. And I don't know what side-effects could cause it to other hooks. So I thought better stay at one point where only one addon could get broken instead of brick the whole hook-mechanism ;-)

MrPetovan commented

2019-12-23 01:27:37 +01:00

(Migrated from github.com)

And we don't need it at other places, so why should we store this hook?

Imagine if we decide to delete tables absent from the definition? The addon table would be wiped out the next time the DB update routine is called.

A middle of the road approach would be to keep the register call and just add the add call.

> And we don't need it at other places, so why should we store this hook? Imagine if we decide to delete tables absent from the definition? The addon table would be wiped out the next time the DB update routine is called. A middle of the road approach would be to keep the register call and just add the add call.

👍 1

nupplaphil commented

2019-12-23 01:36:45 +01:00

(Migrated from github.com)

And we don't need it at other places, so why should we store this hook?

Imagine if we decide to delete tables absent from the definition? The addon table would be wiped out the next time the DB update routine is called.

A middle of the road approach would be to keep the register call and just add the add call.

Fair enough

> > And we don't need it at other places, so why should we store this hook? > > Imagine if we decide to delete tables absent from the definition? The addon table would be wiped out the next time the DB update routine is called. > > A middle of the road approach would be to keep the register call and just add the add call. Fair enough

❤️ 1

MrPetovan (Migrated from github.com) approved these changes 2019-12-23 01:37:46 +01:00

No reviewers

MrPetovan