Change in app hook resolution

Currently, app hook resolution isn’t well defined and practically depends on the order defined in apps.txt file. This order isn’t meaningful much, what most people expect the order to be is “App installed on site” i.e. newest app installed on site should override older apps (which are usually core apps like Frappe, ERPNext)

This issue is well defined here: App Hooks Resolution · Issue #17762 · frappe/frappe · GitHub
We are planning to fix it in this PR: refactor!: deprecate sorting based on `apps.txt` in `get_installed_apps` by sagarvora · Pull Request #17869 · frappe/frappe · GitHub

This should be considered a general norm now: “Any business logic that needs to override something or define the order of extensions should consider installed-on-site ordering” If there are any inconsistencies, we will try to address them as soon as possible.

With the PRs linked above the following behaviour will be changed:

  1. App hooks resolution
  2. Templates resolution (jinja templates)

We intend to backport these changes to v14 and v13 too over time (within ~2 weeks) even if they are mildly breaking as they bring some order into otherwise random behaviour.

If you have any suggestions please drop them on github issue.

22 Likes

The last writer’s win is excellent & expected as well.

2 Likes

Thank @ankush for this works !

I’ve got a related problem in the past, and solve it in custom way, nice to see it will be a core feature

good work guys, this will solve many problems, also i was thinking about something like WordPress hooks weight where the hooks with less weight (a number) called/added first

Technically, the problem was not a bug. The fix is also not a refactor since it changes the behavior of the framework plus its a breaking change. I suggest that there should be no backport as this has the potential to affect everyone that has had some form of work-around. In worst case scenarios, it will introduce bugs that may go undetected by users. Please don’t backport a breaking change :pray:

Asides that, :+1:

1 Like

I may agree with you on this for version 13. But with version 14 we have moved away from monolith architecture so this needs to be back-ported to v14 at least where the order of app installation should decide the winner. Yes, this may require some people to fix things but that’s ok because this is the correct way of doing things IMHO. And v14 is going to remain in production for quite some time.

It may introduce bugs for some users which may go undetected by some users. I believe that is a good enough reason to give deep consideration to a backport especially since its not a bug fix (which qualifies for a back port based on policy).
I don’t think the added convenience has more value than the potential to cause problems for the small subset of users that may be affected; some customisation are built based on the current behavior and in some cases, taking advantage of the current behavior (<= v14 behavior). Majority of users will not notice the change but lets not make it inherently painful for those that will.
Finally, its not really a monolith issue as the change is on the framework not an app.

Just my opinion.

1 Like

This order isn’t meaningful much, what most people expect the order to be is “App installed on site” i.e. newest app installed on site should override older apps (which are usually core apps like Frappe, ERPNext)

@ankush this also applies to translations. Currently it’s not possibly to overwrite ERPNext’s de.csv with my custom app’s de.csv (except, maybe, by changing the order in apps.txt)

@rmeyer we should switch to same logic for translations too. last write wins should be common and expected behaviour wherever such problem arises (unless a better solution is being proposed?)

@tundebabzy I understand the concerns, but bringing order in randomness is hardly a “breaking change” (can we break something that is already broken?)

Also I’ve added a small UI feature to adjust the order if required on existing sites. You can re-order it without doing anything crazy. But be cautious while using this because you can very well break something else while trying to fix one thing.

image

5 Likes

We need to formalize our policy on breaking changes, cause while this is breaking change it’s just become essential to add in stable versions. FWIW I don’t think we have promised to follow of SemVer strictly ever, but it is valuable for the community so for the most part we have followed it.

4 Likes

Thank you Sagar and Ankush. A much needed fix

1 Like

This change is now also applied to translation files, we’ll ship the fix in 1-2 weeks to stable branches.

3 Likes