Page MenuHomePhabricator

Add validation for config settings of type regex
ClosedPublic

Authored by amckinley on Apr 13 2017, 8:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 21, 12:17 AM
Unknown Object (File)
Thu, Nov 21, 12:16 AM
Unknown Object (File)
Thu, Nov 21, 12:16 AM
Unknown Object (File)
Thu, Nov 21, 12:01 AM
Unknown Object (File)
Tue, Nov 19, 11:11 PM
Unknown Object (File)
Sat, Nov 16, 10:00 AM
Unknown Object (File)
Mon, Nov 11, 9:39 PM
Unknown Object (File)
Fri, Nov 8, 5:12 AM

Details

Summary

Also fixes insufficiently-escaped regex examples

Test Plan

Made several changes to http://local.phacility.com/config/edit/syntax.filemap/ and observed validation failures on malformed regexes, and success on well-formed regexes.

Diff Detail

Repository
rP Phabricator
Branch
T12532
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 16460
Build 21911: Run Core Tests
Build 21910: arc lint + arc unit

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.
src/applications/config/custom/PhabricatorConfigRegexOptionType.php
4

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

8

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.