Summary:
This addresses three issues with the current patch management system:
# Two people developing at the same time often pick the same SQL patch number, and then have to go rename it. The system catches this, but it's silly.
# Second/third-party developers can't use the same system to manage auxiliary storage they may want to add.
# There's no way to build mock databases for unit tests that need to do reads.
To resolve these things, you can now name your patches whatever you want and conflicts are just merge conflicts, which are less of a pain to fix than filename conflicts.
Dependencies are now a DAG, with implicit dependencies created on the prior patch if no dependencies are specified. Developers can add new concrete subclasses of `PhabricatorSQLPatchList` to add storage management, and define the dependency branchpoint of their patches so they apply in the correct order (although, generally, they should not depend on the mainline patches, presumably).
The commands `storage upgrade --namespace test1234` and `storage destroy --namespace test1234` will allow unit tests to build and destroy MySQL storage.
A "quickstart" mode allows an upgrade from scratch in ~1200ms. Destruction takes about 200ms. These seem like fairily reasonable costs to actually use in tests. Building from scratch patch-by-patch takes about 6000ms.
Test Plan:
- Created new databases from scratch with and without quickstart in a separate test namespace. Pointed the webapp at the test namespaces, browsed around, everything looked good.
- Compared quickstart and no-quickstart dump states, they're identical except for mysqldump timestamps and a few similar things.
- Upgraded a legacy database to the new storage format.
- Destroyed / dumped storage.
Reviewers: edward, vrana, btrahan, jungejason
Reviewed By: btrahan
CC: aran, nh
Maniphest Tasks: T140, T345
Differential Revision: https://secure.phabricator.com/D2323
Summary: See T993. MySQL fails very very softly if you request an engine which does not exist. Detect and fail if the InnoDB engine is missing or broken.
Test Plan: Faked InnoDB missing, got a failure. Ran normally, got success.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T993
Differential Revision: https://secure.phabricator.com/D2160
Summary: This separates common MySQL stuff (identifiers and comments escaping, error codes, connection retries) from PHP extension specific stuff (connect, query, fetch, errors, escape string).
Test Plan:
/
Use `AphrontMySQLiDatabaseConnection` in `PhabricatorLiskDAO`, load homepage, edit task, save task.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran
Differential Revision: https://secure.phabricator.com/D2113
Test Plan:
Run setup with 'differential.attach-task-class' set to:
- ''
- 'FacebookTasksAttacher'
- 'X'
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1999
Summary:
1. The setup flow complains if you haven't updated your schema, so that section
should be moved above the setup flow.
2. The setup flow tells you to lower your timeout, but it doesn't tell you how
low will make it stop complaining.
Test Plan:
Didn't test the setup.
Regenerated the docs and saw the change.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1888
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary:
I accidentally added two "104" patches. This actually works OK for the most part
but is fundamentally bad and wrong.
Merge the patches (installs applied both as "104", so we can't move one to
"105") and add a safeguard.
Test Plan: Ran upgrade_schema.php with two "104" patches, got error'd. Ran
without, got successs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1614
"Content-Disposition: attachment"
Summary:
We currently serve some files off the primary domain (with "Content-Disposition:
attachment" + a CSRF check) and some files off the alternate domain (without
either).
This is not sufficient, because some UAs (like the iPad) ignore
"Content-Disposition: attachment". So there's an attack that goes like this:
- Alice uploads xss.html
- Alice says to Bob "hey download this file on your iPad"
- Bob clicks "Download" on Phabricator on his iPad, gets XSS'd.
NOTE: This removes the CSRF check for downloading files. The check is nice to
have but only raises the barrier to entry slightly. Between iPad / sniffing /
flash bytecode attacks, single-domain installs are simply insecure. We could
restore the check at some point in conjunction with a derived authentication
cookie (i.e., a mini-session-token which is only useful for downloading files),
but that's a lot of complexity to drop all at once.
(Because files are now authenticated only by knowing the PHID and secret key,
this also fixes the "no profile pictures in public feed while logged out"
issue.)
Test Plan: Viewed, info'd, and downloaded files
Reviewers: btrahan, arice, alok
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T843
Differential Revision: https://secure.phabricator.com/D1608
Summary:
By default, PHP-FMP (an alternate PHP FCGI SAPI) cleans the entire environment
for child processes. This means we have no $PATH.
This causes some confusing failures for reasons I don't fully understand. If you
do these things:
exec_manual('env');
exec_manual('export');
...they show no $PATH, as expected. If you do this:
exec_manual('echo $PATH');
...it shows a path. And this works (i.e., it finds the executable):
exec_manual('ls');
...but this fails (it says "no ls in ((null))"):
exec_manual('which ls');
So, basically, the sh -c process itself gets a default PATH somehow, but its
children don't. I don't realllly get why this happens, but clearly an empty
$PATH is a misconfiguration, and can easily be remedied.
See discussion here: https://github.com/facebook/libphutil/issues/7
Test Plan: Applied patch to Centos6 + nginx + PHP-FPM machine, ran setup, the
configuration issue was detected and I was given information on resolving it.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1413
Summary:
- PHP uses a SAPI ("server API") to determine how it interacts with the caller
(e.g., how to read the environment, how to read flags, what code to execute).
- There are several different SAPIs: cli, cgi, cgi-fcgi, apache, etc.
- Each SAPI has different behavior -- for instance, the "cgi" SAPI emits some
CGI headers unless told not to, so a script like 'echo "x"' actually echoes some
headers and then 'x' as an HTTP body.
- In some setups, "php" may be php-cgi.
- If you run php-cgi as "php scriptname.php" and your ENV has an existing CGI
request in it, it runs that CGI request instead of the script. This causes an
infinite loop.
- Add checks to verify that "php" is the "cli" SAPI binary, not some other
SAPI.
- In particular, cPanel uses suphp and is affected by this configuration
issue. See this thread:
https://lists.marsching.com/pipermail/suphp/2008-September/002036.html
Test Plan:
- On a cPanel + suphp machine, ran setup and was stopped for having the
"cgi-fcgi" SAPI instead of throw into an infinite loop.
- Applied the suggested remedy, setup now runs fine.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1390
Summary:
With T764, http://localhost doesn't work anymore. So add instructions
about how to support it by modifying the hosts file.
Test Plan:
- turned on setup mode and the error message did show up
- turned off the setup mode and the error message also showed up
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T764
Differential Revision: https://secure.phabricator.com/D1370
Summary:
Chrome/Chromium won't set cookies on these domains, at least under
Ubuntu. See T754. Detect brokenness and explode.
Test Plan:
Logged into phabricator as "http://derps/" (failed) and
"http://derps.com/" (worked) in Chromium. Set config to "http://derps/" (config
exploded) and "http://local.aphront.com/" (config OK).
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T754
Differential Revision: https://secure.phabricator.com/D1355
Summary: a well-titled diff this be. i feel 'meh' about the change; doesn't
seem to help too much imo.
Test Plan:
edited my custom conf file to have errors -
127.0.0.1 => 127.0.0.2
mysql_user => mysql_users
and for phabricator to be in setup mode. for each error i verified that i liked
the display.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1129
setup
Summary: See T481. We'll fail the pcntl test if we don't have this, in a
potentially confusing way. Test and detect missing 'php' explicitly before we
try the pcntl test, so we can give the user a better error message.
Test Plan: In setup mode, did a good run and then faked it to execute 'phpx'
instead to get a failure.
Reviewers: johnduhart, jungejason, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 878
Summary: This may make it easier to debug problems with CLI + pcntl
Test Plan: Added a syntax error to the script and got more useful output
Reviewers: johnduhart, jungejason, tuomaspelkonen, aran
Reviewed By: johnduhart
CC: aran, johnduhart
Differential Revision: 869
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
Summary: This didn't end up getting used but I neglected to delete it.
Test Plan: git grep
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, jungejason
Differential Revision: 721
problems
Summary: Detect more PHP misconfigurations in setup.
Test Plan: Broke my configuration, ran setup, it seemed to detect all the
problems and issue meaningful error messages.
Reviewed By: jungejason
Reviewers: hunterbridges, 10098, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 717
Summary:
See T344. Currently, there's a hard-coded 12MB filesize limit and some awkward
interactions with MySQL's max_allowed_packet. Make this system generally more
robust:
- Move the upload limit to configuration.
- Add setup steps which reconcile max_allowed_packet vs MySQL file storage
limits.
- Add a layer of indirection between uploading files and storage engines.
- Allow the definition of new storage engines.
- Define a local disk storage engine.
- Add a "storage engine selector" class which manages choosing which storage
engines to put files in.
- Document storage engines.
- Document file storage classes.
Test Plan:
Setup mode:
- Disabled MySQL storage engine, misconfigured it, configured it correctly.
- Disabled file storage engine, set it to something invalid, set it to
something valid.
- Verified max_allowed_packet is read correctly.
Application mode:
- Configured local file storage.
- Uploaded large and small files.
- Verified larger files were written to local storage.
- Verified smaller files were written to MySQL blob storage.
Documentation:
- Read documentation.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 695
Summary:
A bunch of the .sql patch files don't explicitly specify the table engine, but
we should always use InnoDB with the exception of one table which needs MyISAM
for FULLTEXT.
MySQL doesn't no-op an ALTER TABLE statment that changes the engine back to
itself and converting large tables can be time consuming, so convert only the
required tables.
Test Plan: Ran on secure.phabricator.com and my local box, it fixed all the
issues in about 3 seconds on secure.phabricator.com and <<1 second on my local.
Reviewed By: codeblock
Reviewers: codeblock, tuomaspelkonen, jungejason, aran
CC: aran, epriestley, codeblock
Differential Revision: 641
Summary:
If we're going to hardcode a path, at least let's do it in a way that works on RHEL too.
Test Plan:
Successfully ran the setup script on RHEL.
Reviewers:
epriestley
CC:
Differential Revision: 623
Summary:
The MySQL MyISAM fulltext backend defaults to a minimum length of 4, but many
3-letter words are useful search terms. Provide instructions for configuring
MySQL to reduce the auto-stopword length.
Test Plan:
Followed instructions, searched for a 3-letter term and got a hit.
Reviewed By: jungejason
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, jungejason
Differential Revision: 471
Summary:
remove accessing the db config info directly. Use
DatabaseConfigurationProvider instead. Also fixed a minor issue where
different number of newlines are output in PhabricatorSetup.php's output.
Test Plan:
executed upgrade_schema.php; executed PhabricatorSetup.php by
setting 'phabricator.setup' to true.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 443
Summary:
Make 'gd' an explicit optional dependency, test for it in setup, and make the
software behave correctly if it is not available.
When generating file thumnails, provide reasonable defaults and behavior for
non-image files.
Test Plan:
Uploaded text files, pdf files, etc., and got real thumbnails instead of a
broken image.
Simulated setup and gd failures and walked through setup process and image
fallback for thumbnails.
Reviewed By: aran
Reviewers: toulouse, jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 446
Summary:
- Make the instructional text generally more useful.
- Show the current configured adapter.
- When the configuration prevents outbound email from being delivered, show a
warning.
- Detect 'curl' extension during setup since it's more-or-less required
- Add curl extension to the install scripts
codeblock: can you verify the rhel-derivs changes are correct?
Test Plan:
Set adapter to test, verified warning; entered setup mode and verified curl. Ran
apt-get on an ubuntu box. Ran yum on an amazon linux box.
Reviewed By: toulouse
Reviewers: toulouse, codeblock
Commenters: codeblock
CC: aran, jungejason, tuomaspelkonen, codeblock, epriestley, toulouse
Differential Revision: 438
Summary:
There was a last-minute edit to this to fix a typo before rP089d8327 landed
which accidentally made it impossible to pass the check. :)
Test Plan:
Put install into setup mode, changed protocol to 'http', 'ftp'.
Reviewed By: cadamo
Reviewers: toulouse, codeblock, cadamo
Commenters: toulouse, codeblock
CC: aran, cadamo, toulouse, codeblock
Differential Revision: 434
Summary:
Well, since I couldn't regenerate my arcanist cert I figured out that this wass because "workflows" are unavailable there now. I really can not figure out why but it was.
I added in the setup script, the ability to check if is present the protocol of the host and if it has a trailing slash a the end of the line, since both are needed to generate the cert.
Users now only be able to upload valid image files with mimetype of jpg, jpeg,
png and gif.
Test Plan:
FIRST: DO NOT apply those changes! then
1- go to settings->arcanist certificate and the click on regenerate ... humm
2- On your config file, delete the trailing slash at the end and the protocol on "phabricator.base-uri", then go to setting->arcanist certificate. Here you
will see something like this "phabricator.example.comapi\/" instead of
"http:\/\/phabricator.example.com\/api\/".
SECOND: Now apply this changes:
1- Go to settings->arcanist certificate and the click on regenerate.
2- On your config file, delete the trailing slash at the end and the protocol
on "phabricator.base-uri", and setup "phabricator.setup" to true.
3- Then go to setting->arcanist certificate and you could see that this was successfully generated.
THIRD:
Go to settings->account and try to upload an invalid image file, and do the same on "youruserna"->edit profile.
Reviewed By: epriestley
Reviewers: epriestley jungejason
CC: epriestley jugesason cadamo aran
Differential Revision: 391
Summary:
In RHEL6 at the least, pcntl installs from distro package management to the CLI
but not to Apache. Since we don't need it in apache and it's a pain to build
manually, just verify it exists on the CLI.
Test Plan:
Simulated script failures and verified setup output.
Reviewed By: codeblock
Reviewers: codeblock, aran, jungejason, tuomaspelkonen
CC: aran, epriestley, kevinwallace, codeblock
Differential Revision: 380
Summary:
Removes calling `which` and instead checks the file path that PHPMailerLite uses directly (/usr/bin/sendmail).
This fixes $PATH export issues which occur on certain platforms causing un-expected errors during setup.
Test Plan:
* Run setup on server without sendmail install & error should be presented.
* Install sendmail then re-run setup & no error should be presented.
Reviewers: epriestley
Differential Revision: 375
Summary:
After successfully installing phabricator on my Mac OS X 10.6.7, I was unable to
link my accounts to either Facebook or GitHub.
I diagnosed that file_get_contents() and fopen() were not working properly.
After installing the php openssl package I was able to get it linking
successfully.
Test Plan:
With php's openssl extension disabled, and phabricator installed. Try linking to
Facebook and GitHub and observe that it fails. You can visit the Auth
Diagnostics page and "Facebook Graph" and "App Login" should fail.
With php's openssl extension enabled, linking to Facebook and GitHub should be
successful.
Change the configuration to add "phabricator.setup = false".
Disable php's openssl extension. Visit the phabricator site and observe that it
requires you to install php's openssl extension.
Enable php's openssl extension. Visit the phabricator site and observe that it
installs fine.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 352
Summary:
ccheever did an install and gave me some feedback about issues he hit. This
tries to:
- properly document how to configure outbound email;
- test outbound email configuration in the setup mode;
- provide basic daemon documentation;
- document that phabricator.base-uri is required for all installs.
Test Plan:
read documentation, jumped through all the setup branches to test configuration
error detection
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran, rm
CC: ccheever, aran
Differential Revision: 276
Summary:
Alters the installation instructions to guide installers into a "setup" mode
which does config file sanity checking.
Test Plan:
Put myself in setup mode, simulated all the failures it detects, took myself out
of setup mode, Phabricator works OK.
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 230
