Not sure if we want this, but it seems to work fine.
- Restricted Diffusion Commit
rPbb58a123e633: Modularize Celerity postprocessors
The only other thing I can think of is letting people pull in an actual CSS file or define their own rules, but I suppose that is actual theming and this is just to make Phabricayor easier to read.
A couple of really nitpicky inlines. There is some inconsistency in the use of postprocessor versus postProcessor, personally I feel that the latter is more correct, but you seem to have used the former in more places.
I feel like this method should be named setPostprocessorKey or that it should actually take an instance of CelerityPostprocessor.
Maybe name this $postprocessor for consistency with CelerityResourceTransformer
As above, I feel that this would be more correctly named setPostprocessorKey
You maybe got a bit carried away here :P
Maybe move this to CelerityPostprocessor as in PhabricatorEdgeType and PhabricatorPHIDType.
Maybe add a basic test class as in D13272. I haven't seen these tests catch any real issues yet, but theoretically it means that we will catch issues with two postprocessors having the same key.
I wonder if the "return the default postprocessor" logic should live here? Like, maybe CelerityPostprocessor::getPostprocessor(null) should just return the default. Also, maybe CelerityPostprocessor::getPostprocessor('nonexistent_key') should throw an exception
I've been wondering if we should make these fields final? This is probably more siginificant if/when we drop PHP 5.2 support and can use late static binding.
- Use Postprocessor consistently (I started with "PostProcessor" but it looked super weird after I typed it 5 times so I switched, but didn't clean them all up).
- Remove ocean of whitespace between tiny island methods.
- Added tests for getAll() not hitting key collisions.
- Marked static methods final.
- Use postprocessorKey() when we're passing a key.
- Fix ajax.
- Make high contrast colors more obvious.
I also made "default to DefaultPostprocessor" a rule in the base class. This feels a little dirty (the parent refers to a child symbol by name) but simplifies subclass implementations, and the default postprocessor is more special in this case than in most cases of modular subclasses.
As an aside, I also think we could probably write some kind of shared piece of "turn these objects into a map with unique keys and throw on collision" code, which is getting more common. I think the major difference between them is specializing the strings, but I think the error is usually pretty obvious when you hit it and it would be OK to have more generic strings.