Code review comment for lp:~vauxoo/openobject-server/7.0-custom_domain_method

Hello Jose,

Thank you for proposing this improvement to OpenERP!
I'm afraid we'll have to decline it because this was not the intended behavior for callable o2m domains.

Indeed this feature was introduced in v7 (here: [1]) for the only purpose of supporting static domains on inherited o2m fields where the reverse foreign key is not a true foreign key.
And you're right, this is used by the mail.thread model which is abstract and inherited by many models, and whose `message_ids` field has a composite reverse foreign key in the mail.message table: (res_model, res_id). So this is the reason why the `domain` callable will only receive the model as an argument. It should only be used for similar purposes.

We don't want to allow more complex cases because developers will start to abuse them ;-) This is very frequent in the framework, and then the developers are disappointed when that abusive behavior gets broken by a future patch, because it was not supposed to work. For example if a context is provided in a method, developers will always find a way to implement context-sensitive logic even in places where it should not happen, like workflows or _constraints.

In general you should only use framework features in the way they are used by official addons, unless explicitly documented. If you think you need this feature but you are not in the same case as mail.thread, you should think about a different model/design to accomplish what you need. Otherwise you're probably going to meet other problems in the long run, and this patch is not the solution.

I hope you understand this explanation.

In addition, I have a few technical remarks about your patch:
- This is a wishlist API change, so even if backwards-compatible it will never be accepted in a stable version. You should have proposed it for trunk, not 7.0.
- Your implementation accepts callables with different signatures. This is an anti-pattern and OpenERP is already plagued with such horrors, like the read() and browse() methods that return different types based on the IDs/ID passed. This forces *every* piece of code that deals with that API to check the actual signature, adding a lot of useless code everywhere. We would like to avoid this in the future.
- You only patched 1 out of the 3 places where domain callables are managed, so this patch will not properly work. Search fields.py for 'self._domain(' to see all of them.

Thanks!

[1] server 7.0 revno 4333.1.3 revid:<email address hidden>