Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HookBundle] Review if hook system can be simplified #4404

Closed
Guite opened this issue Jul 14, 2020 · 4 comments · Fixed by #4593
Closed

[HookBundle] Review if hook system can be simplified #4404

Guite opened this issue Jul 14, 2020 · 4 comments · Fixed by #4593
Labels
Milestone

Comments

@Guite
Copy link
Member

Guite commented Jul 14, 2020

The current hook system is very inconsistent and therefore hard to understand.

One potentially confusing thing is that UI/display hooks are rendered outside of the main form, so if they provide something and rely on that being submitted it won't. So UI hooks mainly make sense for TYPE_DISPLAY_VIEW, the other six subtypes like DISPLAY_EDIT are very rarely needed (only if a hook provider wants to validate and process the subscriber data instead of providing additional form elements).

So a typical hook provider (which offers and processes additional data) currently needs to offer both form aware and UI hook providers for a "complete" experience (adding custom form elements and displaying assigned data on view pages).
Also a site admin must know this and assign both providers to get things working.

Hence, we should rethink about whether it is possible to simplify the different hook types and unify how they are implemented.

@Guite Guite added the Refactor label Jul 14, 2020
@Guite Guite added this to the 4.0.0 milestone Jul 14, 2020
@craigh
Copy link
Member

craigh commented Jul 14, 2020

Of course, the original hooks implementation was designed by Drak when we used "regular" forms (and FormUtil 😱) and so the UiHooks category of hooks is old and not designed as symfony forms aware.

This is why with Core-2 I added the FormAware hooks category. These are intentionally designed for use in a Symfony Form aware workflow. FormAware hooks replace the 6 UiHooks related to form workflows. This leaves only \Zikula\Bundle\HookBundle\Category\UiHooksCategory::TYPE_DISPLAY_VIEW

probably we should immediately deprecate the "old 6" UiHooks and remove at 4.0. The old "manual" method of handling forms would not be 'hookable'. (probably this should have been done before Core-3 🙄)

This would leave us with 3 categories:

  1. FilterHook (only 1 type: filter)
  2. FormAwareHook (4 types: edit, delete, process_edit, process_delete because form validation is automatic)
  3. UiHook (only 1 type: display) maybe this category could be renamed

(remember, that an extension developer may implement and utilize their own hook categories)

From there, I believe implementation is simpler. We would need to better document the intention of each category.

  1. Filter: filter content before display
  2. FormAware: amend/complement the form workflow
  3. Display: display additional content for a model.
  • It is worth noting that a module like EZComments is a Display hook provider that will use its own form workflow to display comments. It is NOT a FormAware category provider.
  • Scribite is a FormAware provider that does not amend or utilize any form data
  • Profile is a FormAware provider that actually amends the User forms to amend the form and add/store its own related data.

Hence, we should rethink about whether it is possible to simplify the different hook types and unify how they are implemented

Unify is an interesting word-choice here. I do not think we need to unify the categories (e.g. have only 1 category). Perhaps we can better document them in a more unified way or present a better DX.

refs #4398, #4296, #3944 (#3983)

@craigh
Copy link
Member

craigh commented Jul 14, 2020

ping @paustian Since you have utilized hooks quite a lot, I wonder if you have any thoughts.
Also @Kaik I know you have used many hooks also.

@Guite
Copy link
Member Author

Guite commented Jul 14, 2020

Unify was related to the implementation, i.e. same arguments and equal naming etc.

@Guite Guite changed the title Review if hook system can be simplified [HookBundle] Review if hook system can be simplified Jul 15, 2020
@Kaik
Copy link
Contributor

Kaik commented Aug 4, 2020

Hooks were always confusing maybe because of my idea how it supposed to work.

So for example to create BBSmiles functionality via hooks you have to create two hook providers BBSmiles Display hook (FormAware is not needed as we would use js to replace smiles in textarea) and BBSmiles Filter hook.

For some reason there is no one "plane" on which an area is connected with its categories. It seems category is more important than area.

Current provider id provider.dizkus.filter_hooks.bbcode
My provider id provider.dizkus.bbcode.filter_hooks

Current subscriber id subscriber.news.filter_hooks.content
My provider id subscriber.news.content.filter_hooks

So this way when I bind one bbcode provider to an "content" area all categories are matched automatically.

I actually do not understand why some "operations on the content" are distinctive categories and others dont not. For example why filtering has its own category while validating or processing does not have it...

Maybe there is a deeper reason why it is the way it is...

One thing that I think is really missing is ability to add settings for a binding. So each connection have its own settings specified by hook provider. This way we one provider can act differently when hooked to a different subscriber.

@craigh craigh linked a pull request Feb 7, 2021 that will close this issue
9 tasks
@Guite Guite modified the milestones: 4.0.0, 3.1.0 Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants