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

App Hooks Resolution #17762

Closed
gavindsouza opened this issue Aug 9, 2022 · 2 comments · Fixed by #17869
Closed

App Hooks Resolution #17762

gavindsouza opened this issue Aug 9, 2022 · 2 comments · Fixed by #17869

Comments

@gavindsouza
Copy link
Collaborator

gavindsouza commented Aug 9, 2022

Description of the issue

Execution of the hooks installed on a particular site follows the alphabetical order of their naming doesn't follow a well-defined order. If an app that starts with the letter 'a''s hooks will always be executed first. There's no other control provided for priority resolution for hooks. If you want an app to override some hook with the highest priority, you'll have to change your app's name!

We discussed possible ways of handling resolution: By order of app installation on a particular site was one that seemed the most obvious quick fix for this broken system. According to this, the last installed app would have the highest priority. For hooks that pick only a single value from a mapping like override_whitelisted_methods or override_doctype_class, the last installed app that defines said hook value would be picked.

Although this adds more sanity to the system, it still feels lacking. We might need an interface for tracking hooks priority and overriding them. This should be at a site level given the timing of apps being installed on benches is immaterial and only when or how (dependency install, or explicit, etc) the app is installed on a site matters for context.

Addtional Context: #16246, #12075

@DrZoidberg09
Copy link
Contributor

First of all, thank you for the explanation of how the order of hooks is sorted. This now explains why the hooks I use in my app (starting with a C) are then overwritten by ERPNext, if they also exist in the ERPNext hooks.py.

I would certainly agree that the order of installed apps would be a good fix, as this also defines the order of how migrate work.

Ultimately, it would be absolutely great to have a function that defines the order of hooks (and migrate). But having it by the order of install would be already awesome!

Looking forward to see this function! 👍

@ankush
Copy link
Member

ankush commented Jan 13, 2023

Can be fixed by #17869

We are going with "installed on site" order, last install wins.

We will introduce UI to change order if required in future.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants