Page MenuHomePhabricator

Add validation for config settings of type regex

Authored by amckinley on Apr 13 2017, 8:52 PM.



Also fixes insufficiently-escaped regex examples

Test Plan

Made several changes to and observed validation failures on malformed regexes, and success on well-formed regexes.

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Apr 13 2017, 8:53 PM
This revision was automatically updated to reflect the committed changes.
joshuaspence added inline comments.

Shouldn't this be final by preference? I thought there was a linter rule for this.


What if $value isn't an array?

Oh, good catches:

  • With very rare exceptions, classes should either be abstract or final. Earlier in the life of the project we didn't do this and a few users decided that meant that they were free to extend any class they wanted in third-party custom code and complain when we broke things. With final, they have to more clearly "break the warranty seal" by modifying the upstream code, and we haven't seen further confusion about this that I can recall since we adopted the rule.
  • ArcanistRaggedClassTreeEdgeXHPASTLinterRule is supposed to enforce this in lint, but is disabled by default and not enabled by the phutil.xhpast ruleset. I'll file a task to fix this.
  • validateOption() can do more complete validation by doing this check first, which makes sure users don't enter a value like "asdf". This might not be permitted by the web UI anyway, but users can write "syntax.filemap": "asdf" into the local.json config if they're ambitious:
if (!is_array($value)) {
  throw new Exception(...);

I'll file a task to fix [ragged classtree linting].

Filed as T12555.