diff --git a/src/docs/book/contributor.book b/src/docs/book/contributor.book new file mode 100644 index 0000000000..2960d4e439 --- /dev/null +++ b/src/docs/book/contributor.book @@ -0,0 +1,33 @@ +{ + "name" : "phabcontrib", + "title" : "Phabricator Contributor Documentation", + "short" : "Phabricator Contributor Docs", + "preface" : "Information for Phabricator contributors.", + "root" : "../../../", + "uri.source" : + "https://secure.phabricator.com/diffusion/P/browse/master/%f$%l", + "rules" : { + "(\\.diviner$)" : "DivinerArticleAtomizer" + }, + "exclude" : [ + "(^externals/)", + "(^webroot/rsrc/externals/)", + "(^scripts/)", + "(^support/)", + "(^resources/)", + "(^src/docs/user/)", + "(^src/docs/tech/)", + "(^src/docs/flavor/)" + ], + "groups" : { + "contrib" : { + "name" : "Contributor Overview" + }, + "standards" : { + "name" : "Coding Standards" + }, + "developer" : { + "name" : "Developer Guides" + } + } +} diff --git a/src/docs/book/flavor.book b/src/docs/book/flavor.book new file mode 100644 index 0000000000..92e5158ece --- /dev/null +++ b/src/docs/book/flavor.book @@ -0,0 +1,42 @@ +{ + "name" : "phabflavor", + "title" : "Phabricator Flavor Text", + "short" : "Flavor Text", + "preface" : "Recommendations, lore, and dark rituals.", + "root" : "../../../", + "uri.source" : + "https://secure.phabricator.com/diffusion/P/browse/master/%f$%l", + "rules" : { + "(\\.diviner$)" : "DivinerArticleAtomizer" + }, + "exclude" : [ + "(^externals/)", + "(^webroot/rsrc/externals/)", + "(^scripts/)", + "(^support/)", + "(^resources/)", + "(^src/docs/user/)", + "(^src/docs/tech/)", + "(^src/docs/contributor/)" + ], + "groups" : { + "overview" : { + "name" : "Overview" + }, + "review" : { + "name" : "Revision Control and Code Review" + }, + "sundry" : { + "name" : "Sundries" + }, + "lore" : { + "name" : "Phabricator Lore" + }, + "php" : { + "name" : "PHP" + }, + "javascript" : { + "name" : "Javascript" + } + } +} diff --git a/src/docs/book/phabricator.book b/src/docs/book/phabricator.book index 7b8c794d6b..cf363a647d 100644 --- a/src/docs/book/phabricator.book +++ b/src/docs/book/phabricator.book @@ -1,271 +1,273 @@ { "name" : "phabdev", "title" : "Phabricator Technical Documentation", "short" : "Phabricator Tech Docs", "preface" : "Technical documentation intended for Phabricator developers.", "root" : "../../../", "uri.source" : "https://secure.phabricator.com/diffusion/P/browse/master/%f$%l", "rules" : { "(\\.php$)" : "DivinerPHPAtomizer", "(\\.diviner$)" : "DivinerArticleAtomizer" }, "exclude" : [ "(^externals/)", "(^webroot/rsrc/externals/)", "(^scripts/)", "(^support/)", "(^resources/)", - "(^src/docs/user/)" + "(^src/docs/user/)", + "(^src/docs/flavor/)", + "(^src/docs/contributor/)" ], "groups" : { "arcanist" : { "name" : "Arcanist Integration", "include" : "(^src/applications/arcanist/)" }, "audit" : { "name" : "Audit", "include" : "(^src/applications/audit/)" }, "auth" : { "name" : "Auth", "include" : "(^src/applications/auth/)" }, "baseapp" : { "name" : "Application Basics", "include" : "(^src/applications/base/)" }, "cache" : { "name" : "Cache", "include" : "(^src/applications/cache/)" }, "calendar" : { "name" : "Calendar", "include" : "(^src/applications/calendar/)" }, "chatlog" : { "name" : "Chatlog", "include" : "(^src/applications/chatlog/)" }, "conduit" : { "name" : "Conduit", "include" : "(^src/applications/conduit/)" }, "config" : { "name" : "Config", "include" : "(^src/applications/config/)" }, "conpherence" : { "name" : "Conpherence", "include" : "(^src/applications/conpherence/)" }, "countdown" : { "name" : "Countdown", "include" : "(^src/applications/countdown/)" }, "daemon" : { "name" : "Daemons", "include" : "(^src/applications/daemon/)" }, "differential" : { "name" : "Differential", "include" : "(^src/applications/differential/)" }, "diffusion" : { "name" : "Diffusion", "include" : "(^src/applications/diffusion/)" }, "directory" : { "name" : "Directory", "include" : "(^src/applications/directory/)" }, "diviner" : { "name" : "Diviner", "include" : "(^src/applications/diviner/)" }, "doorkeeper" : { "name" : "Doorkeeper", "include" : "(^src/applications/doorkeeper/)" }, "draft" : { "name" : "Draft", "include" : "(^src/applications/draft/)" }, "drydock" : { "name" : "Drydock", "include" : "(^src/applications/drydock/)" }, "fact" : { "name" : "Fact", "include" : "(^src/applications/fact/)" }, "feed" : { "name" : "Feed", "include" : "(^src/applications/feed/)" }, "files" : { "name" : "Files", "include" : "(^src/applications/files/)" }, "flag" : { "name" : "Flags", "include" : "(^src/applications/flag/)" }, "harbormaster" : { "name" : "Harbormaster", "include" : "(^src/applications/harbormaster/)" }, "help" : { "name" : "Help", "include" : "(^src/applications/help/)" }, "herald" : { "name" : "Herald", "include" : "(^src/applications/herald/)" }, "legalpad" : { "name" : "Legalpad", "include" : "(^src/applications/legalpad/)" }, "lipsum" : { "name" : "Lipsum", "include" : "(^src/applications/lipsum/)" }, "macro" : { "name" : "Macro", "include" : "(^src/applications/macro/)" }, "mailinglists" : { "name" : "Mailing Lists", "include" : "(^src/applications/mailinglists/)" }, "maniphest" : { "name" : "Maniphest", "include" : "(^src/applications/maniphest/)" }, "meta" : { "name" : "Meta", "include" : "(^src/applications/meta/)" }, "metamta" : { "name" : "MetaMTA", "include" : "(^src/applications/metamta/)" }, "notification" : { "name" : "Notifications", "include" : "(^src/applications/notification/)" }, "oauthserver" : { "name" : "OAuth Server", "include" : "(^src/applications/oauthserver/)" }, "owners" : { "name" : "Owners", "include" : "(^src/applications/owners/)" }, "paste" : { "name" : "Paste", "include" : "(^src/applications/paste/)" }, "people" : { "name" : "People", "include" : "(^src/applications/people/)" }, "phame" : { "name" : "Phame", "include" : "(^src/applications/phame/)" }, "phid" : { "name" : "PHIDs", "include" : "(^src/applications/phid/)" }, "phlux" : { "name" : "Phlux", "include" : "(^src/applications/phlux/)" }, "pholio" : { "name" : "Pholio", "include" : "(^src/applications/pholio/)" }, "phortune" : { "name" : "Phortune", "include" : "(^src/applications/phortune/)" }, "phpast" : { "name" : "PHPAST", "include" : "(^src/applications/phpast/)" }, "phrequent" : { "name" : "Phrequent", "include" : "(^src/applications/phrequent/)" }, "phriction" : { "name" : "Phriction", "include" : "(^src/applications/phriction/)" }, "policy" : { "name" : "Policy", "include" : "(^src/applications/policy/)" }, "ponder" : { "name" : "Ponder", "include" : "(^src/applications/ponder/)" }, "project" : { "name" : "Projects", "include" : "(^src/applications/project/)" }, "releeph" : { "name" : "Releeph", "include" : "(^src/applications/releeph/)" }, "remarkup" : { "name" : "Remarkup", "include" : "(^src/applications/remarkup/)" }, "repository" : { "name" : "Repositories", "include" : "(^src/applications/repository/)" }, "search" : { "name" : "Search", "include" : "(^src/applications/search/)" }, "settings" : { "name" : "Settings", "include" : "(^src/applications/settings/)" }, "slowvote" : { "name" : "Slowvote", "include" : "(^src/applications/slowvote/)" }, "subscriptions" : { "name" : "Subscriptions", "include" : "(^src/applications/subscriptions/)" }, "system" : { "name" : "System", "include" : "(^src/applications/system/)" }, "tokens" : { "name" : "Tokens", "include" : "(^src/applications/tokens/)" }, "transactions" : { "name" : "Transactions", "include" : "(^src/applications/transactions/)" }, "typeahead" : { "name" : "Typeahead", "include" : "(^src/applications/typeahead/)" }, "uiexample" : { "name" : "UI Examples", "include" : "(^src/applications/uiexample/)" }, "xhprof" : { "name" : "XHProf", "include" : "(^src/applications/xhprof/)" } } } diff --git a/src/docs/book/user.book b/src/docs/book/user.book index 96c7df40a2..e05fb21f20 100644 --- a/src/docs/book/user.book +++ b/src/docs/book/user.book @@ -1,49 +1,51 @@ { "name" : "phabricator", "title" : "Phabricator User Documentation", "short" : "Phabricator User Docs", "preface" : "Instructions for installing, configuring, and using Phabricator.", "root" : "../../../", "uri.source" : "https://secure.phabricator.com/diffusion/P/browse/master/%f$%l", "rules" : { "(\\.diviner$)" : "DivinerArticleAtomizer" }, "exclude" : [ "(^externals/)", "(^webroot/rsrc/externals/)", "(^scripts/)", "(^support/)", "(^resources/)", - "(^src/docs/tech/)" + "(^src/docs/tech/)", + "(^src/docs/flavor/)", + "(^src/docs/contributor/)" ], "groups" : { "intro" : { "name" : "Introduction" }, "config" : { "name" : "Configuration" }, "userguide" : { "name" : "Application User Guides" }, "differential" : { "name" : "Differential (Code Review)" }, "diffusion" : { "name" : "Diffusion (Repository Browser)" }, "maniphest" : { "name" : "Maniphest (Task Tracking)" }, "slowvote" : { "name" : "Slowvote (Polls)" }, "herald" : { "name" : "Herald (Notifications)" }, "phriction" : { "name" : "Phriction (Wiki)" } } } diff --git a/src/docs/user/developer/adding_new_css_and_js.diviner b/src/docs/contributor/adding_new_css_and_js.diviner similarity index 100% rename from src/docs/user/developer/adding_new_css_and_js.diviner rename to src/docs/contributor/adding_new_css_and_js.diviner diff --git a/src/docs/user/contributing/contrib_intro.diviner b/src/docs/contributor/contrib_intro.diviner similarity index 100% rename from src/docs/user/contributing/contrib_intro.diviner rename to src/docs/contributor/contrib_intro.diviner diff --git a/src/docs/user/developer/darkconsole.diviner b/src/docs/contributor/darkconsole.diviner similarity index 100% rename from src/docs/user/developer/darkconsole.diviner rename to src/docs/contributor/darkconsole.diviner diff --git a/src/docs/user/developer/database.diviner b/src/docs/contributor/database.diviner similarity index 100% rename from src/docs/user/developer/database.diviner rename to src/docs/contributor/database.diviner diff --git a/src/docs/user/contributing/general_coding_standards.diviner b/src/docs/contributor/general_coding_standards.diviner similarity index 99% rename from src/docs/user/contributing/general_coding_standards.diviner rename to src/docs/contributor/general_coding_standards.diviner index 0ed00f98d3..a2294d0e10 100644 --- a/src/docs/user/contributing/general_coding_standards.diviner +++ b/src/docs/contributor/general_coding_standards.diviner @@ -1,147 +1,147 @@ @title General Coding Standards -@group contrib +@group standards This document is a general coding standard for contributing to Phabricator, Arcanist, libphutil and Diviner. = Overview = This document contains practices and guidelines which apply across languages. Contributors should follow these guidelines. These guidelines are not hard-and-fast but should be followed unless there is a compelling reason to deviate from them. = Code Complexity = - Prefer to write simple code which is easy to understand. The simplest code is not necessarily the smallest, and some changes which make code larger (such as decomposing complex expressions and choosing more descriptive names) may also make it simpler. Be willing to make size tradeoffs in favor of simplicity. - Prefer simple methods and functions which take a small number of parameters. Avoid methods and functions which are long and complex, or take an innumerable host of parameters. When possible, decompose monolithic, complex methods into several focused, simpler ones. - Avoid putting many ideas on a single line of code. For example, avoid this kind of code: COUNTEREXAMPLE $category_map = array_combine( $dates, array_map(create_function('$z', 'return date("F Y", $z);'), $dates)); Expressing this complex transformation more simply produces more readable code: $category_map = array(); foreach ($dates as $date) { $category_map[$date] = date('F Y', $date); } And, obviously, don't do this sort of thing: COUNTEREXAMPLE if ($val = $some->complicatedConstruct() && !!~blarg_blarg_blarg() & $flags ? HOPE_YOU_MEMORIZED == $all_the_lexical_binding_powers : <<<'Q' ${hahaha} Q ); = Performance = - Prefer to write efficient code. - Strongly prefer to drive optimization decisions with hard data. Avoid optimizing based on intuition or rumor if you can not support it with concrete measurements. - Prefer to optimize code which is slow and runs often. Optimizing code which is fast and runs rarely is usually a waste of time, and can even be harmful if it makes that code more difficult to understand or maintain. You can determine if code is fast or slow by measuring it. - Reject performance discussions that aren't rooted in concrete data. In Phabricator, you can usually use the builtin XHProf profiling to quickly gather concrete performance data. = Naming Things = - Follow language-specific conventions. - Name things unambiguously. - Choose descriptive names. - Avoid nonstandard abbreviations (common abbreviations like ID, URI and HTTP are fine). - Spell words correctly. - Use correct grammar. For example, avoid these sorts of naming choices: COUNTEREXAMPLE $PIE->GET_FLAVOR(); // Unconventional. $thing->doStuff(); // Ambiguous. $list->empty(); // Ambiguous -- is it isEmpty() or makeEmpty()? $e = 3; // Not descriptive. $this->updtHndlr(); // Nonstandard abbreviation. $this->chackSpulls(); // Misspelling, ungrammatical. Prefer these: $pie->getFlavor(); // Conventional. $pie->bake(); // Unambiguous. $list->isEmpty(); // Unambiguous. $list->makeEmpty(); // Unambiguous. $edge_count = 3; // Descriptive. $this->updateHandler(); // No nonstandard abbreviations. $this->getID(); // Standard abbreviation. $this->checkSpelling(); // Correct spelling and grammar. = Error Handling = - Strongly prefer to detect errors. - Strongly prefer to fail fast and loudly. The maximum cost of script termination is known, bounded, and fairly small. The maximum cost of continuing script execution when errors have occurred is unknown and unbounded. This also makes APIs much easier to use and problems far easier to debug. When you ignore errors, defer error handling, or degrade the severity of errors by treating them as warnings and then dismissing them, you risk dangerous behavior which may be difficult to troubleshoot: COUNTEREXAMPLE exec('echo '.$data.' > file.bak'); // Bad! do_something_dangerous(); exec('echo '.$data.' > file.bak', $out, $err); // Also bad! if ($err) { debug_rlog("Unable to copy file!"); } do_something_dangerous(); Instead, fail loudly: exec('echo '.$data.' > file.bak', $out, $err); // Better if ($err) { throw new Exception("Unable to copy file!"); } do_something_dangerous(); But the best approach is to use or write an API which simplifies condition handling and makes it easier to get right than wrong: execx('echo %s > file.bak', $data); // Good do_something_dangerous(); Filesystem::writeFile('file.bak', $data); // Best do_something_dangerous(); See @{article@libphutil:Command Execution} for details on the APIs used in this example. = Documentation, Comments and Formatting = - Prefer to remove code by deleting it over removing it by commenting it out. It shall live forever in source control, and can be retrieved therefrom if it is ever again called upon. - In source code, use only ASCII printable characters plus space and linefeed. Do not use UTF-8 or other multibyte encodings. diff --git a/src/docs/user/contributing/internationalization.diviner b/src/docs/contributor/internationalization.diviner similarity index 99% rename from src/docs/user/contributing/internationalization.diviner rename to src/docs/contributor/internationalization.diviner index 56f7ac221d..25d790e808 100644 --- a/src/docs/user/contributing/internationalization.diviner +++ b/src/docs/contributor/internationalization.diviner @@ -1,64 +1,64 @@ @title Internationalization -@group contrib +@group developer What is required from developers to get Phabricator translatable. = API = Translator API is provided by libphutil. It gives us @{class@libphutil:PhutilTranslator} class and global @{function@libphutil:pht} function built on top of it. Developers are supposed to call @{function@libphutil:pht} on all strings that require translation. Phabricator provides translations for this translator through @{class:PhabricatorTranslation} class. = Adding a New Translation = Adding a translation which uses the same language rules as some already existing translation is relatively simple: Just extend @{class:PhabricatorTranslation} and you will be able to specify this class in the global configuration 'translation.provider' and users will be able to select it in their preferences. = Adding a New Language = Adding a language involves all steps as adding a translation plus specifying the language rules in @{method@libphutil:PhutilTranslator::chooseVariant}. = Singular and Plural = Different languages have various rules for using singular and plural. All you need to do is to call @{function@libphutil:pht} with a text that is suitable for both forms. Example: pht('%d beer(s)', $count); Translators will translate this text for all different forms the language uses: // English translation array('%d beer', '%d beers'); // Czech translation array('%d pivo', '%d piva', '%d piv'); The ugly identifier passed to @{function@libphutil:pht} will remain in the text only if the translation doesn't exist. = Male and Female = Different languages use different words for talking about males, females and unknown genders. Callsites have to call @{function@libphutil:pht} passing @{class:PhabricatorUser} (or other implementation of @{interface@libphutil:PhutilPerson}) if talking about the user. Example: pht('%s wrote', $actor); Translators will create this translations: // English translation '%s wrote'; // Czech translation array('%s napsal', '%s napsala'); diff --git a/src/docs/user/contributing/javascript_coding_standards.diviner b/src/docs/contributor/javascript_coding_standards.diviner similarity index 99% rename from src/docs/user/contributing/javascript_coding_standards.diviner rename to src/docs/contributor/javascript_coding_standards.diviner index 8634e39caf..83772918c3 100644 --- a/src/docs/user/contributing/javascript_coding_standards.diviner +++ b/src/docs/contributor/javascript_coding_standards.diviner @@ -1,140 +1,140 @@ @title Javascript Coding Standards -@group contrib +@group standards This document describes Javascript coding standards for Phabricator and Javelin. = Overview = This document outlines technical and style guidelines which are followed in Phabricator and Javelin. Contributors should also follow these guidelines. Many of these guidelines are automatically enforced by lint. These guidelines are essentially identical to the Facebook guidelines, since I basically copy-pasted them. If you are already familiar with the Facebook guidelines, you can probably get away with skimming this document. = Spaces, Linebreaks and Indentation = - Use two spaces for indentation. Don't use literal tab characters. - Use Unix linebreaks ("\n"), not MSDOS ("\r\n") or OS9 ("\r"). - Use K&R style braces and spacing. - Put a space after control keywords like ##if## and ##for##. - Put a space after commas in argument lists. - Put space around operators like ##=##, ##<##, etc. - Don't put spaces after function names. - Parentheses should hug their contents. - Generally, prefer to wrap code at 80 columns. = Case and Capitalization = The Javascript language unambiguously dictates casing/naming rules; follow those rules. - Name variables using ##lowercase_with_underscores##. - Name classes using ##UpperCamelCase##. - Name methods and properties using ##lowerCamelCase##. - Name global functions using ##lowerCamelCase##. Avoid defining global functions. - Name constants using ##UPPERCASE##. - Write ##true##, ##false##, and ##null## in lowercase. - "Internal" methods and properties should be prefixed with an underscore. For more information about what "internal" means, see **Leading Underscores**, below. = Comments = - Strongly prefer ##//## comments for making comments inside the bodies of functions and methods (this lets someone easily comment out a block of code while debugging later). = Javascript Language = - Use ##[]## and ##{}##, not ##new Array## and ##new Object##. - When creating an object literal, do not quote keys unless required. = Examples = **if/else:** lang=js if (x > 3) { // ... } else if (x === null) { // ... } else { // ... } You should always put braces around the body of an if clause, even if it is only one line. Note that operators like ##>## and ##===## are also surrounded by spaces. **for (iteration):** lang=js for (var ii = 0; ii < 10; ii++) { // ... } Prefer ii, jj, kk, etc., as iterators, since they're easier to pick out visually and react better to "Find Next..." in editors. **for (enumeration):** lang=js for (var k in obj) { // ... } Make sure you use enumeration only on Objects, not on Arrays. For more details, see @{article:Javascript Object and Array}. **switch:** lang=js switch (x) { case 1: // ... break; case 2: if (flag) { break; } break; default: // ... break; } ##break## statements should be indented to block level. If you don't push them in, you end up with an inconsistent rule for conditional ##break## statements, as in the ##2## case. If you insist on having a "fall through" case that does not end with ##break##, make it clear in a comment that you wrote this intentionally. For instance: lang=js switch (x) { case 1: // ... // Fall through... case 2: //... break; } = Leading Underscores = By convention, methods names which start with a leading underscore are considered "internal", which (roughly) means "private". The critical difference is that this is treated as a signal to Javascript processing scripts that a symbol is safe to rename since it is not referenced outside the current file. The upshot here is: - name internal methods which shouldn't be called outside of a file's scope with a leading underscore; and - **never** call an internal method from another file. If you treat them as though they were "private", you won't run into problems. diff --git a/src/docs/user/developer/n_plus_one.diviner b/src/docs/contributor/n_plus_one.diviner similarity index 100% rename from src/docs/user/developer/n_plus_one.diviner rename to src/docs/contributor/n_plus_one.diviner diff --git a/src/docs/user/developer/phabricator_code_layout.diviner b/src/docs/contributor/phabricator_code_layout.diviner similarity index 100% rename from src/docs/user/developer/phabricator_code_layout.diviner rename to src/docs/contributor/phabricator_code_layout.diviner diff --git a/src/docs/user/contributing/php_coding_standards.diviner b/src/docs/contributor/php_coding_standards.diviner similarity index 99% rename from src/docs/user/contributing/php_coding_standards.diviner rename to src/docs/contributor/php_coding_standards.diviner index cb3406e15b..78021a2f9f 100644 --- a/src/docs/user/contributing/php_coding_standards.diviner +++ b/src/docs/contributor/php_coding_standards.diviner @@ -1,169 +1,169 @@ @title PHP Coding Standards -@group contrib +@group standards This document describes PHP coding standards for Phabricator and related projects (like Arcanist and libphutil). = Overview = This document outlines technical and style guidelines which are followed in libphutil. Contributors should also follow these guidelines. Many of these guidelines are automatically enforced by lint. These guidelines are essentially identical to the Facebook guidelines, since I basically copy-pasted them. If you are already familiar with the Facebook guidelines, you probably don't need to read this super thoroughly. = Spaces, Linebreaks and Indentation = - Use two spaces for indentation. Don't use tab literal characters. - Use Unix linebreaks ("\n"), not MSDOS ("\r\n") or OS9 ("\r"). - Use K&R style braces and spacing. - Put a space after control keywords like ##if## and ##for##. - Put a space after commas in argument lists. - Put a space around operators like ##=##, ##<##, etc. - Don't put spaces after function names. - Parentheses should hug their contents. - Generally, prefer to wrap code at 80 columns. = Case and Capitalization = - Name variables and functions using ##lowercase_with_underscores##. - Name classes using ##UpperCamelCase##. - Name methods and properties using ##lowerCamelCase##. - Use uppercase for common acronyms like ID and HTML. - Name constants using ##UPPERCASE##. - Write ##true##, ##false## and ##null## in lowercase. = Comments = - Do not use "#" (shell-style) comments. - Prefer "//" comments inside function and method bodies. = PHP Language Style = - Use "" tag. - Prefer casts like ##(string)## to casting functions like ##strval()##. - Prefer type checks like ##$v === null## to type functions like ##is_null()##. - Avoid all crazy alternate forms of language constructs like "endwhile" and "<>". - Always put braces around conditional and loop blocks. = PHP Language Features = - Use PHP as a programming language, not a templating language. - Avoid globals. - Avoid extract(). - Avoid eval(). - Avoid variable variables. - Prefer classes over functions. - Prefer class constants over defines. - Avoid naked class properties; instead, define accessors. - Use exceptions for error conditions. - Use type hints, use `assert_instances_of()` for arrays holding objects. = Examples = **if/else:** if ($some_variable > 3) { // ... } else if ($some_variable === null) { // ... } else { // ... } You should always put braces around the body of an if clause, even if it is only one line long. Note spaces around operators and after control statements. Do not use the "endif" construct, and write "else if" as two words. **for:** for ($ii = 0; $ii < 10; $ii++) { // ... } Prefer $ii, $jj, $kk, etc., as iterators, since they're easier to pick out visually and react better to "Find Next..." in editors. **foreach:** foreach ($map as $key => $value) { // ... } **switch:** switch ($value) { case 1: // ... break; case 2: if ($flag) { // ... break; } break; default: // ... break; } ##break## statements should be indented to block level. **array literals:** $junk = array( 'nuts', 'bolts', 'refuse', ); Use a trailing comma and put the closing parenthesis on a separate line so that diffs which add elements to the array affect only one line. **operators:** $a + $b; // Put spaces around operators. $omg.$lol; // Exception: no spaces around string concatenation. $arr[] = $element; // Couple [] with the array when appending. $obj = new Thing(); // Always use parens. **function/method calls:** // One line eject($cargo); // Multiline AbstractFireFactoryFactoryEngine::promulgateConflagrationInstance( $fuel, $ignition_source); **function/method definitions:** function example_function($base_value, $additional_value) { return $base_value + $additional_value; } class C { public static function promulgateConflagrationInstance( IFuel $fuel, IgnitionSource $source) { // ... } } **class:** class Dog extends Animal { const CIRCLES_REQUIRED_TO_LIE_DOWN = 3; private $favoriteFood = 'dirt'; public function getFavoriteFood() { return $this->favoriteFood; } } diff --git a/src/docs/user/developer/rendering_html.diviner b/src/docs/contributor/rendering_html.diviner similarity index 100% rename from src/docs/user/developer/rendering_html.diviner rename to src/docs/contributor/rendering_html.diviner diff --git a/src/docs/user/developer/running_builtin_php_webserver.diviner b/src/docs/contributor/running_builtin_php_webserver.diviner similarity index 100% rename from src/docs/user/developer/running_builtin_php_webserver.diviner rename to src/docs/contributor/running_builtin_php_webserver.diviner diff --git a/src/docs/user/developer/unit_tests.diviner b/src/docs/contributor/unit_tests.diviner similarity index 100% rename from src/docs/user/developer/unit_tests.diviner rename to src/docs/contributor/unit_tests.diviner diff --git a/src/docs/user/developer/using_edges.diviner b/src/docs/contributor/using_edges.diviner similarity index 100% rename from src/docs/user/developer/using_edges.diviner rename to src/docs/contributor/using_edges.diviner diff --git a/src/docs/user/developer/using_oauthserver.diviner b/src/docs/contributor/using_oauthserver.diviner similarity index 100% rename from src/docs/user/developer/using_oauthserver.diviner rename to src/docs/contributor/using_oauthserver.diviner diff --git a/src/docs/user/flavortext/about_flavor_text.diviner b/src/docs/flavor/about_flavor_text.diviner similarity index 92% rename from src/docs/user/flavortext/about_flavor_text.diviner rename to src/docs/flavor/about_flavor_text.diviner index 17cc8d317b..3d46ff41d0 100644 --- a/src/docs/user/flavortext/about_flavor_text.diviner +++ b/src/docs/flavor/about_flavor_text.diviner @@ -1,9 +1,9 @@ @title About Flavor Text -@group flavortext +@group overview Explains what's going on here. = Overview = Flavor Text is a collection of short articles which pertain to software development in general, not necessarily to Phabricator specifically. diff --git a/src/docs/user/flavortext/javascript_object_array.diviner b/src/docs/flavor/javascript_object_array.diviner similarity index 99% rename from src/docs/user/flavortext/javascript_object_array.diviner rename to src/docs/flavor/javascript_object_array.diviner index fde8b33ecf..cf7e3371e6 100644 --- a/src/docs/user/flavortext/javascript_object_array.diviner +++ b/src/docs/flavor/javascript_object_array.diviner @@ -1,152 +1,152 @@ @title Javascript Object and Array -@group flavortext +@group javascript This document describes the behaviors of Object and Array in Javascript, and a specific approach to their use which produces basically reasonable language behavior. = Primitives = Javascript has two native datatype primitives, Object and Array. Both are classes, so you can use ##new## to instantiate new objects and arrays: COUNTEREXAMPLE var a = new Array(); // Not preferred. var o = new Object(); However, **you should prefer the shorthand notation** because it's more concise: lang=js var a = []; // Preferred. var o = {}; (A possible exception to this rule is if you want to use the allocation behavior of the Array constructor, but you almost certainly don't.) The language relationship between Object and Array is somewhat tricky. Object and Array are both classes, but "object" is also a primitive type. Object is //also// the base class of all classes. lang=js typeof Object; // "function" typeof Array; // "function" typeof {}; // "object" typeof []; // "object" var a = [], o = {}; o instanceof Object; // true o instanceof Array; // false a instanceof Object; // true a instanceof Array; // true = Objects are Maps, Arrays are Lists = PHP has a single ##array## datatype which behaves like as both map and a list, and a common mistake is to treat Javascript arrays (or objects) in the same way. **Don't do this.** It sort of works until it doesn't. Instead, learn how Javascript's native datatypes work and use them properly. In Javascript, you should think of Objects as maps ("dictionaries") and Arrays as lists ("vectors"). You store keys-value pairs in a map, and store ordered values in a list. So, store key-value pairs in Objects. var o = { // Good, an object is a map. name: 'Hubert', species: 'zebra' }; console.log(o.name); ...and store ordered values in Arrays. var a = [1, 2, 3]; // Good, an array is a list. a.push(4); Don't store key-value pairs in Arrays and don't expect Objects to be ordered. COUNTEREXAMPLE var a = []; a['name'] = 'Hubert'; // No! Don't do this! This technically works because Arrays are Objects and you think everything is fine and dandy, but it won't do what you want and will burn you. = Iterating over Maps and Lists = Iterate over a map like this: lang=js for (var k in object) { f(object[k]); } NOTE: There's some hasOwnProperty nonsense being omitted here, see below. Iterate over a list like this: lang=js for (var ii = 0; ii < list.length; ii++) { f(list[ii]); } NOTE: There's some sparse array nonsense being omitted here, see below. If you try to use ##for (var k in ...)## syntax to iterate over an Array, you'll pick up a whole pile of keys you didn't intend to and it won't work. If you try to use ##for (var ii = 0; ...)## syntax to iterate over an Object, it won't work at all. If you consistently treat Arrays as lists and Objects as maps and use the corresponding iterators, everything will pretty much always work in a reasonable way. = hasOwnProperty() = An issue with this model is that if you write stuff to Object.prototype, it will show up every time you use enumeration ##for##: COUNTEREXAMPLE var o = {}; Object.prototype.duck = "quack"; for (var k in o) { console.log(o[k]); // Logs "quack" } There are two ways to avoid this: - test that ##k## exists on ##o## by calling ##o.hasOwnProperty(k)## in every single loop everywhere in your program and only use libraries which also do this and never forget to do it ever; or - don't write to Object.prototype. Of these, the first option is terrible garbage. Go with the second option. = Sparse Arrays = Another wrench in this mess is that Arrays aren't precisely like lists, because they do have indexes and may be sparse: var a = []; a[2] = 1; console.log(a); // [undefined, undefined, 1] The correct way to deal with this is: for (var ii = 0; ii < list.length; ii++) { if (list[ii] == undefined) { continue; } f(list[ii]); } Avoid sparse arrays if possible. = Ordered Maps = If you need an ordered map, you need to have a map for key-value associations and a list for key order. Don't try to build an ordered map using one Object or one Array. This generally applies for other complicated datatypes, as well; you need to build them out of more than one primitive. diff --git a/src/docs/user/flavortext/javascript_pitfalls.diviner b/src/docs/flavor/javascript_pitfalls.diviner similarity index 99% rename from src/docs/user/flavortext/javascript_pitfalls.diviner rename to src/docs/flavor/javascript_pitfalls.diviner index a4815e8f3a..fb9af3c197 100644 --- a/src/docs/user/flavortext/javascript_pitfalls.diviner +++ b/src/docs/flavor/javascript_pitfalls.diviner @@ -1,87 +1,87 @@ @title Javascript Pitfalls -@group flavortext +@group javascript This document discusses pitfalls and flaws in the Javascript language, and how to avoid, work around, or at least understand them. = Implicit Semicolons = Javascript tries to insert semicolons if you forgot them. This is a pretty horrible idea. Notably, it can mask syntax errors by transforming subexpressions on their own lines into statements with no effect: lang=js string = "Here is a fairly long string that does not fit on one " "line. Note that I forgot the string concatenation operators " "so this will compile and execute with the wrong behavior. "; Here's what ECMA262 says about this: When, as the program is parsed ..., a token ... is encountered that is not allowed by any production of the grammar, then a semicolon is automatically inserted before the offending token if one or more of the following conditions is true: ... To protect yourself against this "feature", don't use it. Always explicitly insert semicolons after each statement. You should also prefer to break lines in places where insertion of a semicolon would not make the unparseable parseable, usually after operators. = ##with## is Bad News = ##with## is a pretty bad feature, for this reason among others: with (object) { property = 3; // Might be on object, might be on window: who knows. } Avoid ##with##. = ##arguments## is not an Array = You can convert ##arguments## to an array using JX.$A() or similar. Note that you can pass ##arguments## to Function.prototype.apply() without converting it. = Object, Array, and iteration are needlessly hard = There is essentially only one reasonable, consistent way to use these primitives but it is not obvious. Navigate these troubled waters with @{article:Javascript Object and Array}. = typeof null == "object" = This statement is true in Javascript: typeof null == 'object' This is pretty much a bug in the language that can never be fixed now. = Number, String, and Boolean objects = Like Java, Javascript has primitive versions of number, string, and boolean, and object versions. In Java, there's some argument for this distinction. In Javascript, it's pretty much completely worthless and the behavior of these objects is wrong. String and Boolean in particular are essentially unusable: lang=js "pancake" == "pancake"; // true new String("pancake") == new String("pancake"); // false var b = new Boolean(false); b; // Shows 'false' in console. !b; // ALSO shows 'false' in console. !b == b; // So this is true! !!b == !b // Negate both sides and it's false! FUCK! if (b) { // Better fucking believe this will get executed. } There is no advantage to using the object forms (the primitive forms behave like objects and can have methods and properties, and inherit from Array.prototype, Number.prototype, etc.) and their logical behavior is at best absurd and at worst strictly wrong. **Never use** ##new Number()##, ##new String()## or ##new Boolean()## unless your Javascript is God Tier and you are absolutely sure you know what you are doing. diff --git a/src/docs/user/flavortext/php_pitfalls.diviner b/src/docs/flavor/php_pitfalls.diviner similarity index 99% rename from src/docs/user/flavortext/php_pitfalls.diviner rename to src/docs/flavor/php_pitfalls.diviner index 81750a3afa..c51b459910 100644 --- a/src/docs/user/flavortext/php_pitfalls.diviner +++ b/src/docs/flavor/php_pitfalls.diviner @@ -1,301 +1,301 @@ @title PHP Pitfalls -@group flavortext +@group php This document discusses difficult traps and pitfalls in PHP, and how to avoid, work around, or at least understand them. = array_merge() in Incredibly Slow When Merging A List of Arrays = If you merge a list of arrays like this: COUNTEREXAMPLE $result = array(); foreach ($list_of_lists as $one_list) { $result = array_merge($result, $one_list); } ...your program now has a huge runtime because it generates a large number of intermediate arrays and copies every element it has previously seen each time you iterate. In a libphutil environment, you can use @{function@libphutil:array_mergev} instead. = var_export() Hates Baby Animals = If you try to var_export() an object that contains recursive references, your program will terminate. You have no chance to intercept or react to this or otherwise stop it from happening. Avoid var_export() unless you are certain you have only simple data. You can use print_r() or var_dump() to display complex variables safely. = isset(), empty() and Truthiness = A value is "truthy" if it evaluates to true in an ##if## clause: $value = something(); if ($value) { // Value is truthy. } If a value is not truthy, it is "falsey". These values are falsey in PHP: null // null 0 // integer 0.0 // float "0" // string "" // empty string false // boolean array() // empty array Disregarding some bizarre edge cases, all other values are truthy. Note that because "0" is falsey, this sort of thing (intended to prevent users from making empty comments) is wrong in PHP: COUNTEREXAMPLE if ($comment_text) { make_comment($comment_text); } This is wrong because it prevents users from making the comment "0". //THIS COMMENT IS TOTALLY AWESOME AND I MAKE IT ALL THE TIME SO YOU HAD BETTER NOT BREAK IT!!!// A better test is probably strlen(). In addition to truth tests with ##if##, PHP has two special truthiness operators which look like functions but aren't: empty() and isset(). These operators help deal with undeclared variables. In PHP, there are two major cases where you get undeclared variables -- either you directly use a variable without declaring it: COUNTEREXAMPLE function f() { if ($not_declared) { // ... } } ...or you index into an array with an index which may not exist: COUNTEREXAMPLE function f(array $mystery) { if ($mystery['stuff']) { // ... } } When you do either of these, PHP issues a warning. Avoid these warnings by using empty() and isset() to do tests that are safe to apply to undeclared variables. empty() evaluates truthiness exactly opposite of if(). isset() returns true for everything except null. This is the truth table: VALUE if() empty() isset() null false true false 0 false true true 0.0 false true true "0" false true true "" false true true false false true true array() false true true EVERYTHING ELSE true false true The value of these operators is that they accept undeclared variables and do not issue a warning. Specifically, if you try to do this you get a warning: COUNTEREXAMPLE if ($not_previously_declared) { // PHP Notice: Undefined variable! // ... } But these are fine: if (empty($not_previously_declared)) { // No notice, returns true. // ... } if (isset($not_previously_declared)) { // No notice, returns false. // ... } So, isset() really means is_declared_and_is_set_to_something_other_than_null(). empty() really means is_falsey_or_is_not_declared(). Thus: - If a variable is known to exist, test falsiness with if (!$v), not empty(). In particular, test for empty arrays with if (!$array). There is no reason to ever use empty() on a declared variable. - When you use isset() on an array key, like isset($array['key']), it will evaluate to "false" if the key exists but has the value null! Test for index existence with array_key_exists(). Put another way, use isset() if you want to type "if ($value !== null)" but are testing something that may not be declared. Use empty() if you want to type "if (!$value)" but you are testing something that may not be declared. = usort(), uksort(), and uasort() are Slow = This family of functions is often extremely slow for large datasets. You should avoid them if at all possible. Instead, build an array which contains surrogate keys that are naturally sortable with a function that uses native comparison (e.g., sort(), asort(), ksort(), or natcasesort()). Sort this array instead, and use it to reorder the original array. In a libphutil environment, you can often do this easily with @{function@libphutil:isort} or @{function@libphutil:msort}. = array_intersect() and array_diff() are Also Slow = These functions are much slower for even moderately large inputs than array_intersect_key() and array_diff_key(), because they can not make the assumption that their inputs are unique scalars as the ##key## varieties can. Strongly prefer the ##key## varieties. = array_uintersect() and array_udiff() are Definitely Slow Too = These functions have the problems of both the ##usort()## family and the ##array_diff()## family. Avoid them. = foreach() Does Not Create Scope = Variables survive outside of the scope of foreach(). More problematically, references survive outside of the scope of foreach(). This code mutates ##$array## because the reference leaks from the first loop to the second: COUNTEREXAMPLE $array = range(1, 3); echo implode(',', $array); // Outputs '1,2,3' foreach ($array as &$value) {} echo implode(',', $array); // Outputs '1,2,3' foreach ($array as $value) {} echo implode(',', $array); // Outputs '1,2,2' The easiest way to avoid this is to avoid using foreach-by-reference. If you do use it, unset the reference after the loop: foreach ($array as &$value) { // ... } unset($value); = unserialize() is Incredibly Slow on Large Datasets = The performance of unserialize() is nonlinear in the number of zvals you unserialize, roughly O(N^2). zvals approximate time 10000 5ms 100000 85ms 1000000 8,000ms 10000000 72 billion years = call_user_func() Breaks References = If you use call_use_func() to invoke a function which takes parameters by reference, the variables you pass in will have their references broken and will emerge unmodified. That is, if you have a function that takes references: function add_one(&$v) { $v++; } ...and you call it with call_user_func(): COUNTEREXAMPLE $x = 41; call_user_func('add_one', $x); ...##$x## will not be modified. The solution is to use call_user_func_array() and wrap the reference in an array: $x = 41; call_user_func_array( 'add_one', array(&$x)); // Note '&$x'! This will work as expected. = You Can't Throw From __toString() = If you throw from __toString(), your program will terminate uselessly and you won't get the exception. = An Object Can Have Any Scalar as a Property = Object properties are not limited to legal variable names: $property = '!@#$%^&*()'; $obj->$property = 'zebra'; echo $obj->$property; // Outputs 'zebra'. So, don't make assumptions about property names. = There is an (object) Cast = You can cast a dictionary into an object. $obj = (object)array('flavor' => 'coconut'); echo $obj->flavor; // Outputs 'coconut'. echo get_class($obj); // Outputs 'stdClass'. This is occasionally useful, mostly to force an object to become a Javascript dictionary (vs a list) when passed to json_encode(). = Invoking "new" With an Argument Vector is Really Hard = If you have some ##$class_name## and some ##$argv## of constructor arguments and you want to do this: new $class_name($argv[0], $argv[1], ...); ...you'll probably invent a very interesting, very novel solution that is very wrong. In a libphutil environment, solve this problem with @{function@libphutil:newv}. Elsewhere, copy newv()'s implementation. = Equality is not Transitive = This isn't terribly surprising since equality isn't transitive in a lot of languages, but the == operator is not transitive: $a = ''; $b = 0; $c = '0a'; $a == $b; // true $b == $c; // true $c == $a; // false! When either operand is an integer, the other operand is cast to an integer before comparison. Avoid this and similar pitfalls by using the === operator, which is transitive. = All 676 Letters in the Alphabet = This doesn't do what you'd expect it to do in C: for ($c = 'a'; $c <= 'z'; $c++) { // ... } This is because the successor to 'z' is 'aa', which is "less than" 'z'. The loop will run for ~700 iterations until it reaches 'zz' and terminates. That is, ##$c## will take on these values: a b ... y z aa // loop continues because 'aa' <= 'z' ab ... mf mg ... zw zx zy zz // loop now terminates because 'zz' > 'z' Instead, use this loop: foreach (range('a', 'z') as $c) { // ... } diff --git a/src/docs/user/flavortext/please_please_please.diviner b/src/docs/flavor/please_please_please.diviner similarity index 98% rename from src/docs/user/flavortext/please_please_please.diviner rename to src/docs/flavor/please_please_please.diviner index 35c83cfa1e..2391a55ba3 100644 --- a/src/docs/user/flavortext/please_please_please.diviner +++ b/src/docs/flavor/please_please_please.diviner @@ -1,78 +1,78 @@ @title Please Please Please -@group flavortext +@group sundry Please read this document. When you send a message that says "I got an error when I tried to do something": = Please Please Please = Please copy and paste the text of the error. Please please please. It is very helpful. So please please please please please please do this. I want to help. I really do. But it is very difficult when you don't include the error text and only allude to how traumatizing reading that text was for you. It is basically inviting me to troll you. I am only human. I have only so much restraint. So please include the error message in your message, before you send it. Or even after you send it, if you realize you forgot. That would be okay too. I would understand. Just please do this. Please. = Thank You = Thank you for reading this note. = Appendix: A Collection of Messages Wherein The Text of the Error is Not Present = Please COUNTEREXAMPLE Subject: help X-Priority: 1 it dun work please COUNTEREXAMPLE Subject: Error I got an error when I ran it. Why? please COUNTEREXAMPLE Subject: error when running program I ran the program and it gave me an error. What do I do now? please COUNTEREXAMPLE Subject: so many errors I got an error and it had instructions in it. I tried to follow the instructions but I got a different error. Are you sure this works? please COUNTEREXAMPLE Subject: confusing error i got an error but it was confusing can you explain what it means? please COUNTEREXAMPLE Subject: Irony There is a spelling error in the error message I received. please include the text of the error message. diff --git a/src/docs/user/flavortext/project_history.diviner b/src/docs/flavor/project_history.diviner similarity index 99% rename from src/docs/user/flavortext/project_history.diviner rename to src/docs/flavor/project_history.diviner index ce4916dbc0..bfdbe2682e 100644 --- a/src/docs/user/flavortext/project_history.diviner +++ b/src/docs/flavor/project_history.diviner @@ -1,60 +1,60 @@ @title Phabricator Project History -@group flavortext +@group lore A riveting tale of adventure. In this document, I refer to worldly and sophisticated engineer Evan Priestley as "I", which is only natural as I am he. This document is mostly just paragraph after paragraph of self-aggrandizement. = In The Beginning = I wrote the original version of Differential in one night at a Facebook Hackathon in April or May 2007, along with Luke Shepard. I joined the company in April and code review was already an established and mostly-mandatory part of the culture, but it happened over email and was inefficient and hard to keep track of. I remember feeling like I was spending a lot of time waiting for code review to happen, which was a major motivator for building the tool. The original name of the tool was "Diffcamp". Some time earlier there had been an attempt to create a project management tool that was a sort of hybrid between Trac and Basecamp called "Traccamp". Since we were writing the code review tool at the height of the brief popularity Traccamp enjoyed, we integrated and called the new tool Diffcamp even though it had no relation to Basecamp. Traccamp fell by the wayside shortly thereafter and was eventually removed. However, Diffcamp didn't share its fate. We spent some more time working on it and got good enough to win hearts and minds over emailing diffs around and was soon the de facto method of code review at Facebook. = The Long Bloat = For the next two and a half years, Diffcamp grew mostly organically and gained a number of features like inline commenting, CLI support and git support (Facebook was 100% SVN in early 2007 but 90%+ of Engineers worked primarily in git with SVN bridging by 2010). As these patches were contributed pretty much randomly, it also gained a lot of performance problems, usability issues, and bugs. Through 2007 and 2008 I worked mostly on frontend and support infrastructure; among other things, I wrote a static resource management system called Haste. In 2009 I worked on the Facebook Lite site, where I built the Javelin Javascript library and an MVC-flavored framework called Alite. But by early 2010, Diffcamp was in pretty bad shape. Two years of having random features grafted onto it without real direction had left it slow and difficult to use. Internal feedback on the tool was pretty negative, with a lot of complaints about performance and stability. The internal XTools team had made inroads at fixing these problems in late 2009, but they were stretched thin and the tool had become a sprawling landscape of architectural and implementation problems. = Differential = I joined the new Dev Tools team around February 2010 and took over Diffcamp. I renamed it to Differential, moved it to a new Alite-based infrastructure with Javelin, and started making it somewhat less terrible. I eventually wrote Diffusion and build Herald to replace a very difficult-to-use predecessor. These tools were less negatively received than the older versions. By December 2010 I started open sourcing them; Haste became //Celerity// and Alite became //Aphront//. I wrote Maniphest to track open issues with the project in January or February and we open sourced Phabricator in late April, shortly after I left Facebook. diff --git a/src/docs/user/flavortext/recommendations_on_branching.diviner b/src/docs/flavor/recommendations_on_branching.diviner similarity index 99% rename from src/docs/user/flavortext/recommendations_on_branching.diviner rename to src/docs/flavor/recommendations_on_branching.diviner index ea5088a786..38d196ad9b 100644 --- a/src/docs/user/flavortext/recommendations_on_branching.diviner +++ b/src/docs/flavor/recommendations_on_branching.diviner @@ -1,188 +1,188 @@ @title Recommendations on Branching -@group flavortext +@group review Project recommendations on how to organize branches. This document discusses organizing branches in your remote/origin for feature development and release management, not the use of local branches in Git or queues or bookmarks in Mercurial. This document is purely advisory. Phabricator works with a variety of branching strategies, and diverging from the recommendations in this document will not impact your ability to use it for code review and source management. = Overview = This document describes a branching strategy used by Facebook and Phabricator to develop software. It scales well and removes the pain associated with most branching strategies. This strategy is most applicable to web applications, and may be less applicable to other types of software. The basics are: - Never put feature branches in the remote/origin/trunk. - Control access to new features with runtime configuration, not branching. The next sections describe these points in more detail, explaining why you should consider abandoning feature branches and how to build runtime access controls for features. = Feature Branches = Suppose you are developing a new feature, like a way for users to "poke" each other. A traditional strategy is to create a branch for this feature in the remote (say, "poke_branch"), develop the feature on the branch over some period of time (say, a week or a month), and then merge the entire branch back into master/default/trunk when the feature is complete. This strategy has some drawbacks: - You have to merge. Merging can be painful and error prone, especially if the feature takes a long time to develop. Reducing merge pain means spending time merging master into the branch regularly. As branches fall further out of sync, merge pain/risk tends to increase. - This strategy generally aggregates risk into a single high-risk merge event at the end of development. It does this both explicitly (all the code lands at once) and more subtly: since commits on the branch aren't going live any time soon, it's easy to hold them to a lower bar of quality. - When you have multiple feature branches, it's impossible to test interactions between the features until they are merged. - You generally can't A/B test code in feature branches, can't roll it out to a small percentage of users, and can't easily turn it on for just employees since it's in a separate branch. Of course, it also has some advantages: - If the new feature replaces an older feature, the changes can delete the older feature outright, or at least transition from the old feature to the new feature fairly rapidly. - The chance that this code will impact production before the merge is nearly zero (it normally requires substantial human error). This is the major reason to do it at all. Instead, consider abandoning all use of feature branching. The advantages are straightforward: - You don't have to do merges. - Risk is generally spread out more evenly into a large number of very small risks created as each commit lands. - You can test interactions between features in development easily. - You can A/B test and do controlled rollouts easily. But it has some tradeoffs: - If a new feature replaces an older feature, both have to exist in the same codebase for a while. But even with feature branching, you generally have to do almost all this work anyway to avoid situations where you flip a switch and can't undo it. - You need an effective way to control access to features so they don't launch before they're ready. Even with this, there is a small risk a feature may launch or leak because of a smaller human error than would be required with feature branching. However, for most applications, this isn't a big deal. This second point is a must-have, but entirely tractable. The next section describes how to build it, so you can stop doing feature branching and never deal with the pain and risk of merging again. = Controlling Access to Features = Controlling access to features is straightforward: build some kind of runtime configuration which defines which features are visible, based on the tier (e.g., development, testing or production?) code is deployed on, the logged in user, global configuration, random buckets, A/B test groups, or whatever else. Your code should end up looking something like this: if (is_feature_launched('poke')) { show_poke(); } Behind that is some code which knows about the 'poke' feature and can go lookup configuration to determine if it should be visible or not. Facebook has a very sophisticated system for this (called GateKeeper) which also integrates with A/B tests, allows you to define complicated business rules, etc. You don't need this in the beginning. Before GateKeeper, Facebook used a much simpler system (called Sitevars) to handle this. Here are some resources describing similar systems: - There's a high-level overview of Facebook's system in this 2011 tech talk: [[http://techcrunch.com/2011/05/30/facebook-source-code/ | Facebook Push Tech Talk]]. - Flickr described their similar system in a 2009 blog post here: [[http://code.flickr.com/blog/2009/12/02/flipping-out/ | Flickr Feature Flags and Feature Flippers]]. - Disqus described their similar system in a 2010 blog post here: [[http://blog.disqus.com/post/789540337/partial-deployment-with-feature-switches | Disqus Feature Switches]]. - Forrst describes their similar system in a 2010 blog post here: [[http://blog.forrst.com/post/782356699/how-we-deploy-new-features-on-forrst | Forrst Buckets]]. - Martin Fowler discusses these systems in a 2010 blog post here: [[http://martinfowler.com/bliki/FeatureToggle.html | Martin Fowler's FeatureToggle]]. - Phabricator just adds config options but defaults them to off. When developing, we turn them on locally. Once a feature is ready, we default it on. We have a vastly less context to deal with than most projects, however, and sometimes get away with simply not linking new features in the UI until they mature (it's open source anyway so there's no value in hiding things). When building this system there are a few things to avoid, mostly related to not letting the complexity of this system grow too wildly: - Facebook initially made it very easy to turn things on to everyone by accident in GateKeeper. Don't do this. The UI should make it obvious when you're turning something on or off, and default to off. - Since GateKeeper is essentially a runtime business rules engine, it was heavily abused to effectively execute code living in a database. Avoid this through simpler design or a policy of sanity. - Facebook allowed GateKeeper rules to depend on other GateKeeper rules (for example, 'new_profile_tour' is launched if 'new_profile' is launched) but did not perform cycle detection, and then sat on a bug describing how to introduce a cycle and bring the site down for a very long time, until someone introduced a cycle and brought the site down. If you implement dependencies, implement cycle detection. - Facebook implemented some very expensive GateKeeper conditions and was spending 100+ ms per page running complex rulesets to do launch checks for a number of months in 2009. Keep an eye on how expensive your checks are. That said, not all complexity is bad: - Allowing features to have states like "3%" instead of just "on" or "off" allows you to roll out features gradually and watch for trouble (e.g., services collapsing from load). - Building a control panel where you hit "Save" and all production servers immediately reflect the change allows you to quickly turn things off if there are problems. - If you perform A/B testing, integrating A/B tests with feature rollouts is probably a natural fit. - Allowing new features to be launched to all employees before they're launched to the world is a great way to get feedback and keep everyone in the loop. Adopting runtime feature controls increases the risk of features leaking (or even launching) before they're ready. This is generally a small risk which is probably reasonable for most projects to accept, although it might be unacceptable for some projects. There are some ways you can mitigate this risk: - Essentially every launch/leak at Facebook was because someone turned on a feature by accident when they didn't mean to. The control panel made this very easy: features defaulted to "on", and if you tried to do something common like remove yourself from the test group for a feature you could easily launch it to the whole world. Design the UI defensively so that it is hard to turn features on to everyone and/or obvious when a feature is launching and this shouldn't be a problem. - The rest were through CSS or JS changes that mentioned new features being shipped to the client as part of static resource packaging or because the code was just added to existing files. If this is a risk you're concerned about, consider integration with static resource management. In general, you can start with a very simple system and expand it as it makes sense. Even a simple system can let you move away from feature branches. = Next Steps = Continue by: - reading recommendations on structuring revision control with @{article:Recommendations on Revision Control}; or - reading recommendations on structuring changes with @{article:Writing Reviewable Code}. diff --git a/src/docs/user/flavortext/recommendations_on_revision_control.diviner b/src/docs/flavor/recommendations_on_revision_control.diviner similarity index 99% rename from src/docs/user/flavortext/recommendations_on_revision_control.diviner rename to src/docs/flavor/recommendations_on_revision_control.diviner index 66b7bfc9d9..0cb0eb5e6c 100644 --- a/src/docs/user/flavortext/recommendations_on_revision_control.diviner +++ b/src/docs/flavor/recommendations_on_revision_control.diviner @@ -1,92 +1,92 @@ @title Recommendations on Revision Control -@group flavortext +@group review Project recommendations on how to organize revision control. This document is purely advisory. Phabricator works with a variety of revision control strategies, and diverging from the recommendations in this document will not impact your ability to use it for code review and source management. This is my (epriestley's) personal take on the issue and not necessarily representative of the views of the Phabricator team as a whole. = Overview = There are a few ways to use SVN, a few ways to use Mercurial, and many many many ways to use Git. Particularly with Git, every project does things differently, and all these approaches are valid for small projects. When projects scale, strategies which enforce **one idea is one commit** are better. = One Idea is One Commit = Choose a strategy where **one idea is one commit** in the authoritative master/remote version of the repository. Specifically, this means that an entire conceptual changeset ("add a foo widget") is represented in the remote as exactly one commit (in some form), not a sequence of checkpoint commits. - In SVN, this means don't ##commit## until after an idea has been completely written. All reasonable SVN workflows naturally enforce this. - In Git, this means squashing checkpoint commits as you go (with ##git commit --amend##) or before pushing (with ##git rebase -i## or ##git merge --squash##), or having a strict policy where your master/trunk contains only merge commits and each is a merge between the old master and a branch which represents a single idea. Although this preserves the checkpoint commits along the branches, you can view master alone as a series of single-idea commits. - In Mercurial, you can use the "queues" extension before 2.2 or `--amend` after Mercurial 2.2, or wait to commit until a change is complete (like SVN), although the latter is not recommended. Without extensions, older versions of Mercurial do not support liberal mutability doctrines (so you can't ever combine checkpoint commits) and do not let you build a default out of only merge commits, so it is not possible to have an authoritative repository where one commit represents one idea in any real sense. = Why This Matters = A strategy where **one idea is one commit** has no real advantage over any other strategy until your repository hits a velocity where it becomes critical. In particular: - Essentially all operations against the master/remote repository are about ideas, not commits. When one idea is many commits, everything you do is more complicated because you need to figure out which commits represent an idea ("the foo widget is broken, what do I need to revert?") or what idea is ultimately represented by a commit ("commit af3291029 makes no sense, what goal is this change trying to accomplish?"). - Release engineering is greatly simplified. Release engineers can pick or drop ideas easily when each idea corresponds to one commit. When an idea is several commits, it becomes easier to accidentally pick or drop half of an idea and end up in a state which is virtually guaranteed to be wrong. - Automated testing is greatly simplified. If each idea is one commit, you can run automated tests against every commit and test failures indicate a serious problem. If each idea is many commits, most of those commits represent a known broken state of the codebase (e.g., a checkpoint with a syntax error which was fixed in the next checkpoint, or with a half-implemented idea). - Understanding changes is greatly simplified. You can bisect to a break and identify the entire idea trivially, without fishing forward and backward in the log to identify the extents of the idea. And you can be confident in what you need to revert to remove the entire idea. - There is no clear value in having checkpoint commits (some of which are guaranteed to be known broken versions of the repository) persist into the remote. Consider a theoretical VCS which automatically creates a checkpoint commit for every keystroke. This VCS would obviously be unusable. But many checkpoint commits aren't much different, and conceptually represent some relatively arbitrary point in the sequence of keystrokes that went into writing a larger idea. Get rid of them or create an abstraction layer (merge commits) which allows you to ignore them when you are trying to understand the repository in terms of ideas (which is almost always). All of these become problems only at scale. Facebook pushes dozens of ideas every day and thousands on a weekly basis, and could not do this (at least, not without more people or more errors) without choosing a repository strategy where **one idea is one commit**. = Next Steps = Continue by: - reading recommendations on structuring branches with @{article:Recommendations on Branching}; or - reading recommendations on structuring changes with @{article:Writing Reviewable Code}. diff --git a/src/docs/user/flavortext/soon_static_resources.diviner b/src/docs/flavor/soon_static_resources.diviner similarity index 99% rename from src/docs/user/flavortext/soon_static_resources.diviner rename to src/docs/flavor/soon_static_resources.diviner index 1a663fea26..78820c5081 100644 --- a/src/docs/user/flavortext/soon_static_resources.diviner +++ b/src/docs/flavor/soon_static_resources.diviner @@ -1,126 +1,126 @@ @title Things You Should Do Soon: Static Resources -@group flavortext +@group sundry Over time, you'll write more JS and CSS and eventually need to put systems in place to manage it. This is part of @{article:Things You Should Do Soon}, which describes architectural problems in web applications which you should begin to consider before you encounter them. = Manage Dependencies Automatically = The naive way to add static resources to a page is to include them at the top of the page, before rendering begins, by enumerating filenames. Facebook used to work like that: COUNTEREXAMPLE true $friend_ids = user_get_friends($user_id); // Get the first 8 friends. $first_few_friends = array_slice($friend_ids, 0, 8); // Render those friends. render_user_friends($user_id, array_keys($first_few_friends)); Because array_slice() in PHP discards array indices and renumbers them, this doesn't render the user's first 8 friends but the users with IDs 0 through 7, e.g. Mark Zuckerberg (ID 4) and Dustin Moskovitz (ID 6). If you have IDs in this range, sooner or later something that isn't an ID will get treated like an ID and the operation will be valid and cause unexpected behavior. This is completely avoidable if you start your IDs at a gigantic number. = Only Store Valid UTF-8 = For the most part, you can ignore UTF-8 and unicode until later. However, there is one aspect of unicode you should address now: store only valid UTF-8 strings. Assuming you're storing data internally as UTF-8 (this is almost certainly the right choice and definitely the right choice if you have no idea how unicode works), you just need to sanitize all the data coming into your application and make sure it's valid UTF-8. If your application emits invalid UTF-8, other systems (like browsers) will break in unexpected and interesting ways. You will eventually be forced to ensure you emit only valid UTF-8 to avoid these problems. If you haven't sanitized your data, you'll basically have two options: - do a huge migration on literally all of your data to sanitize it; or - forever sanitize all data on its way out on the read pathways. As of 2011 Facebook is in the second group, and spends several milliseconds of CPU time sanitizing every display string on its way to the browser, which multiplies out to hundreds of servers worth of CPUs sitting in a datacenter paying the price for the invalid UTF-8 in the databases. You can likely learn enough about unicode to be confident in an implementation which addresses this problem within a few hours. You don't need to learn everything, just the basics. Your language probably already has a function which does the sanitizing for you. = Never Design a Blacklist-Based Security System = When you have an alternative, don't design security systems which are default permit, blacklist-based, or otherwise attempt to enumerate badness. When Facebook launched Platform, it launched with a blacklist-based CSS filter, which basically tried to enumerate all the "bad" parts of CSS and filter them out. This was a poor design choice and lead to basically infinite security holes for all time. It is very difficult to enumerate badness in a complex system and badness is often a moving target. Instead of trying to do this, design whitelist-based security systems where you list allowed things and reject anything you don't understand. Assume things are bad until you verify that they're OK. It's tempting to design blacklist-based systems because they're easier to write and accept more inputs. In the case of the CSS filter, the product goal was for users to just be able to use CSS normally and feel like this system was no different from systems they were familiar with. A whitelist-based system would reject some valid, safe inputs and create product friction. But this is a much better world than the alternative, where the blacklist-based system fails to reject some dangerous inputs and creates //security holes//. It //also// creates product friction because when you fix those holes you break existing uses, and that backward-compatibility friction makes it very difficult to move the system from a blacklist to a whitelist. So you're basically in trouble no matter what you do, and have a bunch of security holes you need to unbreak immediately, so you won't even have time to feel sorry for yourself. Designing blacklist-based security is one of the worst now-vs-future tradeoffs you can make. See also "The Six Dumbest Ideas in Computer Security": http://www.ranum.com/security/computer_security/ = Fail Very Loudly when SQL Syntax Errors Occur in Production = This doesn't apply if you aren't using SQL, but if you are: detect when a query fails because of a syntax error (in MySQL, it is error 1064). If the failure happened in production, fail in the loudest way possible. (I implemented this in 2008 at Facebook and had it just email me and a few other people directly. The system was eventually refined.) This basically creates a high-signal stream that tells you where you have SQL injection holes in your application. It will have some false positives and could theoretically have false negatives, but at Facebook it was pretty high signal considering how important the signal is. Of course, the real solution here is to not have SQL injection holes in your application, ever. As far as I'm aware, this system correctly detected the one SQL injection hole we had from mid-2008 until I left in 2011, which was in a hackathon project on an underisolated semi-production tier and didn't use the query escaping system the rest of the application does. Hopefully, whatever language you're writing in has good query libraries that can handle escaping for you. If so, use them. If you're using PHP and don't have a solution in place yet, the Phabricator implementation of qsprintf() is similar to Facebook's system and was successful there. diff --git a/src/docs/user/flavortext/things_you_should_do_soon.diviner b/src/docs/flavor/things_you_should_do_soon.diviner similarity index 96% rename from src/docs/user/flavortext/things_you_should_do_soon.diviner rename to src/docs/flavor/things_you_should_do_soon.diviner index 3e6a019229..e1d960fd5e 100644 --- a/src/docs/user/flavortext/things_you_should_do_soon.diviner +++ b/src/docs/flavor/things_you_should_do_soon.diviner @@ -1,17 +1,17 @@ @title Things You Should Do Soon -@group flavortext +@group sundry Describes things you should start thinking about soon, because scaling will be easier if you put a plan in place. = Overview = Stop! Don't do any of this yet. Go do @{article:Things You Should Do Now} first. Then you can come back and read about these things. These are basically some problems you'll probably eventually encounter when building a web application that might be good to start thinking about now. = Things You Should Do Soon = - @{article:Things You Should Do Soon: Static Resources} diff --git a/src/docs/user/flavortext/writing_reviewable_code.diviner b/src/docs/flavor/writing_reviewable_code.diviner similarity index 99% rename from src/docs/user/flavortext/writing_reviewable_code.diviner rename to src/docs/flavor/writing_reviewable_code.diviner index 687f7233a2..7e840b7b6d 100644 --- a/src/docs/user/flavortext/writing_reviewable_code.diviner +++ b/src/docs/flavor/writing_reviewable_code.diviner @@ -1,163 +1,163 @@ @title Writing Reviewable Code -@group flavortext +@group review Project recommendations on how to structure changes. This document is purely advisory. Phabricator works with a variety of revision control strategies, and diverging from the recommendations in this document will not impact your ability to use it for code review and source management. = Overview = This document describes a strategy for structuring changes used successfully at Facebook and in Phabricator. In essence: - Each commit should be as small as possible, but no smaller. - The smallest a commit can be is a single cohesive idea: don't make commits so small that they are meaningless on their own. - There should be a one-to-one mapping between ideas and commits: each commit should build one idea, and each idea should be implemented by one commit. - Turn large commits into small commits by dividing large problems into smaller problems and solving the small problems one at a time. - Write sensible commit messages. = Many Small Commits = Small, simple commits are generally better than large, complex commits. They are easier to understand, easier to test, and easier to review. The complexity of understanding, testing and reviewing a change often increases faster than its size: ten 200-line changes each doing one thing are often far easier to understand than one 2,000 line change doing ten things. Splitting a change which does many things into smaller changes which each do only one thing can decrease the total complexity associated with accomplishing the same goal. Each commit should do one thing. Generally, this means that you should separate distinct changes into different commits when developing. For example, if you're developing a feature and run into a preexisting bug, stash or checkpoint your change, check out a clean HEAD/tip, fix the bug in one change, and then merge/rebase your new feature on top of your bugfix so that you have two changes, each with one idea ("add feature x", "fix a bug in y"), not one change with two ideas ("add feature x and fix a bug in y"). (In Git, you can do this easily with local feature branches and commands like `git rebase`, `git rebase -i`, and `git stash`, or with merges. In Mercurial, you can use bookmarks or the queues extension. In SVN, there are few builtin tools, but you can use multiple working copies or treat Differential like a stash you access with `arc patch`.) Even changes like fixing style problems should ideally be separated: they're accomplishing a different goal. And it is far easier to review one 300-line change which "converts tabs to spaces" plus one 30-line change which "implements feature z" than one 330-line change which "implements feature z and also converts a bunch of tabs to spaces". Similarly, break related but complex changes into smaller, simpler components. Here's a ridiculous analogy: if you're adding a new house, don't make one 5,000-line change which adds the whole house in one fell sweep. Split it apart into smaller steps which are each easy to understand: start with the foundation, then build the frame, etc. If you decided to dig the foundation with a shovel or build the frame out of cardboard, it's both easier to miss and harder to correct if the decisions are buried in 5,000 lines of interior design and landscaping. Do it one piece at a time, providing enough context that the larger problem can be understood but accomplishing no more with each step than you need to in order for it to stand on its own. The minimum size of a change should be a complete implementation of the simplest subproblem which works on its own and expresses an entire idea, not just part of an idea. You could mechanically split a 1,000-line change into ten 100-line changes by choosing lines at random, but none of the individual changes would make any sense and you would increase the collective complexity. The real goal is for each change to have minimal complexity, line size is just a proxy that is often well-correlated with complexity. We generally follow these practices in Phabricator. The median change size for Phabricator is 35 lines. See @{article:Differential User Guide: Large Changes} for information about reviewing big checkins. = Write Sensible Commit Messages = There are lots of resources for this on the internet. All of them say pretty much the same thing; this one does too. The single most important thing is: **commit messages should explain //why// you are making the change**. Differential attempts to encourage the construction of sensible commit messages, but can only enforce structure, not content. Structurally, commit messages should probably: - Have a title, briefly describing the change in one line. - Have a summary, describing the change in more detail. - Maybe have some other fields. The content is far more important than the structure. In particular, the summary should explain //why// you're making the change and //why// you're choosing the implementation you're choosing. The //what// of the change is generally well-explained by the change itself. For example, this is obviously an awful commit message: COUNTEREXAMPLE fix a bug But this one is almost as bad: COUNTEREXAMPLE Allow dots in usernames Change the regexps so usernames can have dots in them. This is better than nothing but just summarizes information which can be inferred from the text of the diff. Instead, you should provide context and explain why you're making the change you're making, and why it's the right one: lang=txt Allow dots in usernames to support Google and LDAP auth To prevent nonsense, usernames are currently restricted to A-Z0-9. Now that we have Google and LDAP auth, a couple of installs want to allow "." too since they have schemes like "abraham.lincoln@mycompany.com" (see Tnnn). There are no technical reasons not to do this, so I opened up the regexps a bit. We could mostly open this up more but I figured I'd wait until someone asks before allowing "ke$ha", etc., because I personally find such names distasteful and offensive. This information can not be extracted from the change itself, and is much more useful for the reviewer and for anyone trying to understand the change after the fact. An easy way to explain //why// is to reference other objects (bugs/issues/revisions) which motivate the change. Differential also includes a "Test Plan" field which is required by default. There is a detailed description of this field in @{article:Differential User Guide: Test Plans}. You can make it optional or disable it in the configuration, but consider adopting it. Having this information can be particularly helpful for reviewers. Some things that people sometimes feel strongly about but which are probably not really all that important in commit messages include: - If/where text is wrapped. - Maximum length of the title. - Whether there should be a period or not in the title. - Use of voice/tense, e.g. "fix"/"add" vs "fixes"/"adds". - Other sorts of pedantry not related to getting the context and reasons //why// a change is happening into the commit message. - Although maybe the spelling and grammar shouldn't be egregiously bad? Phabricator does not have guidelines for this stuff. You can obviously set guidelines at your organization if you prefer, but getting the //why// into the message is the most important part. = Next Steps = Continue by: - reading recommendations on structuring revision control with @{article:Recommendations on Revision Control}; or - reading recommendations on structuring branches with @{article:Recommendations on Branching}.