Homec4science

Require valid class for certain config settings

Authored by vrana <jakubv@fb.com> on Mar 15 2012, 20:15.

Description

Require valid class for certain config settings

Summary:
It is now possible to set config setting requiring class of certain implementation to something completely else.
The consequence is that your Phabricator may stop working after update because you didn't implement some new method.

This diff validates the class upon usage.
It throws exception which is better than fatal thrown currently after calling undefined method.

Better solution would be to validate classes when setting the config but it would be too expensive - respective class definitions would have to be loaded and checked by reflection.

I was also thinking about some check script but nobody would run it after changing config.

The same behavior should be implemented for these settings:

  • metamta.mail-adapter
  • metamta.maniphest.reply-handler
  • metamta.differential.reply-handler
  • metamta.diffusion.reply-handler
  • storage.engine-selector
  • search.engine-selector
  • differential.field-selector
  • maniphest.custom-task-extensions-class
  • aphront.default-application-configuration-class
  • controller.oauth-registration

Test Plan:
Send comment, verify that it pass.
Change metamta.differential.reply-handler to incompatible class, verify that sending comment shows nice red exception.
Set metamta.differential.reply-handler to empty string, verify that it throws.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1919

Details

Committed
vrana <jakubv@fb.com>Mar 21 2012, 22:14
Pushed
aubortJan 31 2017, 17:16
Parents
rPHc0aac8267dda: Improve Diffusion behavior for externals
Branches
Unknown
Tags
Unknown

Event Timeline

vrana <jakubv@fb.com> committed rPH07b60b201625: Require valid class for certain config settings (authored by vrana <jakubv@fb.com>).Mar 21 2012, 22:14