Skip to content
This repository
branch: master

Apr 15, 2014

  1. Evan Priestley

    Let project prefilling accept PHIDs

    Summary: I recently made this better about accepting project names, but we use it in some cases with PHIDs. Make that work properly again.
    
    Test Plan: Clicked "New Task" from a project page.
    
    Reviewers: chad, btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Differential Revision: https://secure.phabricator.com/D8778
    authored
  2. Evan Priestley

    Fix a lookup issue in Owners

    Summary:
    Fixes T4477. Sort of winging this but it's probably the right fix?
    
    One error in T4477.
    
    One error via email:
    
    ```
    [2014-04-15 17:44:34] ERROR 8: Undefined index: /some_index/ at [/phab_path/phabricator/src/applications/owners/storage/PhabricatorOwnersPackage.php:213]
      #0 PhabricatorOwnersPackage::findLongestPathsPerPackage(Array of size 3 starting with: { 0 => Array of size 3 starting with: { id => 5 } }, Array of size 8 starting with: { / => Array of size 2 starting with: { /some_index/some_file.py => true } }) called at [/phab_path/phabricator/src/applications/owners/storage/PhabricatorOwnersPackage.php:170]
      #1 PhabricatorOwnersPackage::loadPackagesForPaths(Object PhabricatorRepository, Array of size 2 starting with: { 0 => /some_index/some_file.py }) called at [/phab_path/phabricator/src/applications/owners/storage/PhabricatorOwnersPackage.php:119]
    ...
    ```
    
    Test Plan: Will make @zeeg do it.
    
    Reviewers: btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley, zeeg
    
    Maniphest Tasks: T4477
    
    Differential Revision: https://secure.phabricator.com/D8779
    authored
  3. Bob Trahan

    Maniphest - remove "attach file" action

    Summary: Fixes T4655. Basically leaves the display code intact for legacy installs but removes the option from the UI and removes "create" code.
    
    Test Plan:
    tried to attach file and the action was not in the dropdown!
    made a new task and it worked!
    commented on an old task and it worked!
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    Subscribers: epriestley, Korvin
    
    Maniphest Tasks: T4655
    
    Differential Revision: https://secure.phabricator.com/D8777
    authored
  4. Evan Priestley

    Remove the developer-specific CSRF help in phabricator_form()

    Summary:
    Fixes T4802. For context, see T1921.
    
    Originally (in T1921), a developer ran into an issue where rendering `phabricator_form()` with an absolute URI confusingly dropped CSRF tokens, and it wasn't obvious why. This is a security measure, but at the time it wasn't very clear how all the pieces fit together. To make it more clear, we:
    
      # expanded the exception text in developer mode to include a description of this issue; and
      # added an exception in developer mode when rendering a form like this.
    
    However, (2) causes some undesirable interactions for file downloads. In particular, if:
    
      - developer mode is on; and
      - there's no alternate file domain configured; and
      - you try to download a file...
    
    ...we produce CDN URIs that are fully-qualified, and you get the exception from (2) above.
    
    This is kind of a mess, and producing fully-qualified CDN URIs in all cases is simple and clear and desirable. To resolve this, just revert (2). We still have the clarification from (1) above and this hasn't caused further issues, so I think that's sufficient. This is a rare issue anyway and not particularly serious or error prone (at worst, a bit confusing and annoying, but hopefully easy to understand and resolve after the changes in (1)).
    
    Test Plan: With develper mode and no alternate file domain, downloaded files from Files.
    
    Reviewers: btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Maniphest Tasks: T4802
    
    Differential Revision: https://secure.phabricator.com/D8776
    authored
  5. Evan Priestley

    Make task queue more robust against long-running tasks

    Summary:
    See discussion in D8773. Three small adjustments which should help prevent this kind of issue:
    
      - When queueing followup tasks, hold them on the worker until we finish the task, then queue them only if the work was successful.
      - Increase the default lease time from 60 seconds to 2 hours. Although most tasks finish in far fewer than 60 seconds, the daemons are generally stable nowadays and these short leases don't serve much of a purpose. I think they also date from an era where lease expiry and failure were less clearly distinguished.
      - Increase the default wait-after-failure from 60 seconds to 5 minutes. This largely dates from the MetaMTA era, where Facebook ran services with high failure rates and it was appropriate to repeatedly hammer them until things went through. In modern infrastructure, such failures are rare.
    
    Test Plan:
      - Verified that tasks queued properly after the main task was updated.
      - Verified that leases default to 7200 seconds.
      - Intentionally failed a task and verified default 300 second wait before retry.
      - Removed all default leases shorter than 7200 seconds (there was only one).
      - Checked all the wait before retry implementations for anything much shorter than 5 minutes (they all seem reasonable).
    
    Reviewers: btrahan, sowedance
    
    Reviewed By: sowedance
    
    Subscribers: epriestley
    
    Differential Revision: https://secure.phabricator.com/D8774
    authored

Apr 14, 2014

  1. pengstanford

    Give the commitownersparser a little more time

    Summary:
    Recently we see issues with huge commits (branch cuts for www) where people received hundreds of emails for the same commit. By checking all the active and archived tasks related to such commits, I saw the following pattern:
     - The commit itself is marked as importStatus = 15 which means all the processing was actually done;
     - In archived tasks, I see one PhabricatorRepositorySvnCommitMessageParserWorker, one PhabricatorRepositorySvnCommitChangeParserWorker, followed by many PhabricatorRepositoryCommitHeraldWorker, which means that the PhabricatorRepositoryCommitOwnersWorker (who schedule those herald tasks) was never done;
     - PhabricatorRepositoryCommitOwnersWorker is always active (for days) with failureCount = 0;
     - In daemon log I see a lot of lease expire exception for PhabricatorRepositoryCommitOwnersWorker.
    So to me it looks like the following happened:
     - Everything is fine until we schedule the PhabricatorRepositoryCommitOwnersWorker
     - PhabricatorRepositoryCommitOwnersWorker actually successfully finished but its running time exceed 60s. Before it finishes, it scheduled the PhabricatorRepositoryCommitHeraldWorker task
     - When we try to archive it, the lease expiration exception happened. As a result, it stayed active and will be picked up immediately since it is in the head of the queue
     - The two steps above repeat forever until we kill it
    I am not sure why we want to check lease expiration when we are archiving the task. For now I am giving the worker a little more time since parsing half million affected path needs some time..
    
    Test Plan: Patched in our production and it worked.
    
    Reviewers: lifeihuang, JoelB, #blessed_reviewers, epriestley
    
    Reviewed By: #blessed_reviewers, epriestley
    
    Subscribers: epriestley, Korvin
    
    Differential Revision: https://secure.phabricator.com/D8773
    authored epriestley committed
  2. fcoelho

    Install PHP mbstring extension on RHEL & friends

    Summary:
    The mbstring extension for PHP is not a dependency to any of the already
    listed packages on RHEL-like systems, and is needed by Phabricator
    (showing a "install mbstring" message as the first thing if it is not
    installed)
    
    RHEL seems to have some extra steps to allow php-mbstring to be installed, though:
    
    http://snippets.roozbehk.com/post/35750940300/php-mbstring-missing-on-red-hat-enterprise-linux-6
    
    PS: disabled lint for this change because of the already >80 chars long "yum install" string
    
    Test Plan:
    * Created a new container with docker using both centos:6.4 and fedora:20 images
    * Ran install script
    * Started httpd and mysqld services
    * Browsed to server's address
    * Got error message
    * Installed php-mbstring & restarted httpd
    * Works
    
    Reviewers: #blessed_reviewers, epriestley
    
    Reviewed By: #blessed_reviewers, epriestley
    
    Subscribers: epriestley
    
    Differential Revision: https://secure.phabricator.com/D8772
    authored epriestley committed
  3. Evan Priestley

    Remove some ad-hoc loading of repositories from Releeph

    Summary: Ref T3551. Since we now require repositories in order to perform policy checks, things that did loads properly don't need to load this data explicitly.
    
    Test Plan: Edited a product, cut a new branch.
    
    Reviewers: btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Maniphest Tasks: T3551
    
    Differential Revision: https://secure.phabricator.com/D8769
    authored
  4. Evan Priestley

    Remove several "loadArcanistProject()" methods

    Summary:
    Ref T3551. Releeph has old-style `loadX()` methods; get rid of one of them.
    
    Differential has a couple of copies of this too, clean them up.
    
    Test Plan:
      - Viewed various differential revisions (with and without projects).
      - Viewed and edited Releeph products.
    
    Reviewers: btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Maniphest Tasks: T3551
    
    Differential Revision: https://secure.phabricator.com/D8768
    authored
  5. Evan Priestley

    Unban releeph product name "branch"

    Summary:
    Fixes T3657. We no longer construct ambiguous URIs, so product names are no longer restricted.
    
    Also fix some minor URI construction stuff.
    
    Test Plan: Created a product called "branch".
    
    Reviewers: btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Maniphest Tasks: T3657
    
    Differential Revision: https://secure.phabricator.com/D8767
    authored
  6. Evan Priestley

    Remove ReleephProjectController

    Summary:
    Ref T3657. General changes here:
    
      - Removes `ReleephProjectController`, which is the source of T3657.
      - Mostly moves requests from "RQ" as a monogram to "Y" (looks like a merge, mnemonic for "yank"?, we don't have too many characters left). This should be essentially only cosmetic. This reduces ambiguity with "rQ" and "R123", which are current and future repository monograms. This will continue in the next few diffs.
      - Makes requests implement policies correctly.
    
    Test Plan: Created, edited, browsed requests.
    
    Reviewers: chad, btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Maniphest Tasks: T3657
    
    Differential Revision: https://secure.phabricator.com/D8766
    authored
  7. Evan Priestley

    Add DifferentialHunkQuery to start hiding hunk storage details

    Summary:
    Ref T4045. We have a lot of direct queries against the hunk table right now. These are messy, not really policy-aware, and limit our options on T4045.
    
    This query is unusual (it requires changesets, and does not accept IDs). This keeps us from having to load changeset -> diff -> revision in order to do policy checks. We could also fix this with smarter policy checks and caching, but I'd rather not open that can of worms for now. This object is very low level and relatively unusual, and this small deviation from convention seems like the cleanest cut to make to keep this from snowballing.
    
    Test Plan: Used Herald dry runs to verify that the affected rules still output the same data.
    
    Reviewers: btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Maniphest Tasks: T4045
    
    Differential Revision: https://secure.phabricator.com/D8765
    authored
  8. Evan Priestley

    Simplify Herald logic for loading Differential changes

    Summary: Ref T4045. These three methods are fairly copy-pastey. Provide a more formal DifferentialHunk API for querying various types of line ranges.
    
    Test Plan: Used test console to verify that "added content", "removed content", and "changed content" rules still produce the same data.
    
    Reviewers: btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Maniphest Tasks: T4045
    
    Differential Revision: https://secure.phabricator.com/D8764
    authored

Apr 13, 2014

  1. lkassianik

    Fixing z-index on calendar date badges.

    Summary: Fixes T4787, decreased z-index on calendar badges so that they don't sit on top of  notification dropdowns when dropdowns are expanded. Not sure why the badges had z-index 10, but please let me know if there was a more substantial reason for this.
    
    Test Plan: If neither notification dropdowns have content, create enough messages to populate at least 5 rows, open calendar, expand messages dropdown, verify that underlying calendar date badges do not appear over the dropdown.
    
    Reviewers: chad, #blessed_reviewers, epriestley
    
    Reviewed By: #blessed_reviewers, epriestley
    
    Subscribers: epriestley, Korvin
    
    Maniphest Tasks: T4787
    
    Differential Revision: https://secure.phabricator.com/D8770
    authored epriestley committed

Apr 12, 2014

  1. Evan Priestley

    Move Releeph branch controllers toward a modern/stable state

    Summary:
    Ref T3644. Ref T3657. Ref T3549. Basically:
    
      - Move these controllers to modern query/policy infrastructure.
      - Move them to consistent, ID-based URIs.
      - Rename "Project" to "Product"; "Pick Request" to "Pull Request".
      - Clean up a few UI things here and there.
    
    Test Plan:
      - Created and edited branches.
      - Opened and closed branches.
      - Viewed branch history.
      - Searched within a branch.
      - Browsed to branches from products.
    
    Reviewers: chad, btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Maniphest Tasks: T3644, T3549, T3657
    
    Differential Revision: https://secure.phabricator.com/D8646
    authored
  2. Evan Priestley

    Fixing tooltips not appearing in fullscreen editor

    Summary: Ref T4714, fixing tooltips not appearing in editor when in fullscreen mode
    
    Test Plan: Create paste, verify tooltips appear in comment text-editing bar, make comment box fullscreen, verify tooltips still appear.
    
    Reviewers: chad, #blessed_reviewers, epriestley
    
    Reviewed By: #blessed_reviewers, epriestley
    
    Subscribers: epriestley, Korvin
    
    Maniphest Tasks: T4714
    
    Differential Revision: https://secure.phabricator.com/D8763
    authored

Apr 11, 2014

  1. lkassianik

    Adding getBoundingClientRect math to getPos in Vector.js

    Summary: Ref T4714, tooltips in fullscreen mode need special math, due to fixed position throwing off position of tooltips.
    
    Test Plan: Create work board, create several tasks, create several columns, drag tasks among columns and within columns. Create a paste, check tooltips in comment box show and are positioned correctly. Fullscreen comment box. Verify tooltips still show and position correctly.
    
    Reviewers: epriestley, #blessed_reviewers
    
    Reviewed By: epriestley, #blessed_reviewers
    
    Subscribers: epriestley, Korvin
    
    Maniphest Tasks: T4714
    
    Differential Revision: https://secure.phabricator.com/D8762
    authored epriestley committed
  2. Bob Trahan

    Differential - fix bug writing affected paths

    Summary: Fixes T4774. With the new code and configuration instructions downplaying the role of arcanist project we weren't writing affected paths at all! I had this issue on my installation - no affected paths were written. We seem to always have the repository now though if we can see it, so not too bad of a fix.
    
    Test Plan: updated a diff and was able to browse in diffusion.
    
    Reviewers: epriestley, bitglue
    
    Reviewed By: epriestley, bitglue
    
    Subscribers: bitglue, epriestley, Korvin
    
    Maniphest Tasks: T4774
    
    Differential Revision: https://secure.phabricator.com/D8757
    authored
  3. Bob Trahan

    Differential - make diffs you authored + are reviewer for show up in …

    …appropos boxes
    
    Summary: Fixes T2328. Note the audit part is fixed now.
    
    Test Plan: Tried to reproduce the audit issue by raising my own commit as a problem; it showed up before code changes! Made a diff with my self as author and reviewer; it showed up as expected
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    Subscribers: epriestley, Korvin
    
    Maniphest Tasks: T2328
    
    Differential Revision: https://secure.phabricator.com/D8755
    authored
  4. Evan Priestley

    Minor, fix the scoping of a static variable

    Auditors: chad, btrahan
    authored
  5. Ben Alpert

    Fix typo in variable name

    Summary: Follow-up to D8758.
    
    Test Plan: Crossed fingers.
    
    Reviewers: #blessed_reviewers, chad, epriestley
    
    Reviewed By: #blessed_reviewers, chad, epriestley
    
    Subscribers: epriestley, Korvin
    
    Differential Revision: https://secure.phabricator.com/D8759
    authored epriestley committed
  6. Bob Trahan

    Countdown - use better date control

    Summary: Fixes T3576
    
    Test Plan: made a countdown and it looked right on view. edited it and it had the right values pre and post edit.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    Subscribers: epriestley, Korvin
    
    Maniphest Tasks: T3576
    
    Differential Revision: https://secure.phabricator.com/D8754
    authored
  7. Evan Priestley

    Don't load every commit if there are no local hashes

    Summary: We make a silly query for every commit if you copy/paste a diff.
    
    Test Plan: Copy/pasted diffs now render in fewer than 30 seconds.
    
    Reviewers: btrahan, spicyj
    
    Reviewed By: btrahan, spicyj
    
    Subscribers: epriestley
    
    Differential Revision: https://secure.phabricator.com/D8758
    authored

Apr 10, 2014

  1. Joshua Spence

    Set `celerity.minify` true in production environments.

    Summary: I haven't been able to understand why this isn't set by default in production environments (since it is recommended to do so anyway).
    
    Test Plan: N/A
    
    Reviewers: epriestley, #blessed_reviewers
    
    Reviewed By: epriestley, #blessed_reviewers
    
    Subscribers: epriestley, Korvin
    
    Differential Revision: https://secure.phabricator.com/D8743
    authored epriestley committed
  2. Bob Trahan

    Feed - fix some whacky "text mode" rendering code

    Summary: ...add a "renderingTarget" to FeedStory and use it as appropos. Overall, not a ton of changes was necessary to make this work. I think this could be made to be even cleaner by going through each and every feed story and re-implementing as necessary with the full toolset available. But this is good enough for now I think, and just something to keep cleaning up when we're in here. Fixes T4630.
    
    Test Plan: made a task. gave it a token. viewed my feed - saw stories. used conduit.feed.query with mode == 'text' and got good readable results.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    Subscribers: spicyj, epriestley, Korvin
    
    Maniphest Tasks: T4630
    
    Differential Revision: https://secure.phabricator.com/D8750
    authored
  3. Bob Trahan

    Herald - make tokenizers have the purdy icons

    Summary: ...use the prefab stuff as it does fancier things than we were doing. Only trick then really is to pass username and the map of handle phids => icons to the client so prefab can work nicely. Fixes T4775.
    
    Test Plan: made a herald rule with projects and users. Saw nice icons. Reloaded page and still saw nice icons.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    Subscribers: epriestley, Korvin
    
    Maniphest Tasks: T4775
    
    Differential Revision: https://secure.phabricator.com/D8749
    authored
  4. Evan Priestley

    Modernize chatlog a bit

    Summary:
    Ref T4786. This doesn't fully fix the issue since there's no way to make channels public yet, but gets some of the infrastructure more up to date.
    
      - Allow public access to the list and log controllers.
      - Implement proper policy checks in the Events (this has no practical impact on the only controller that loads this stuff, it's just for general/future purposes).
      - Remove a old-style unused method for building page frames.
    
    Test Plan: Viewed log list and log details as logged-in and logged out users.
    
    Reviewers: btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Maniphest Tasks: T4786
    
    Differential Revision: https://secure.phabricator.com/D8746
    authored
  5. Evan Priestley

    Use better secrets in generating account tokens

    Summary:
    When we generate account tokens for CSRF keys and email verification, one of the inputs we use is the user's password hash. Users won't always have a password hash, so this is a weak input to key generation. This also couples CSRF weirdly with auth concerns.
    
    Instead, give users a dedicated secret for use in token generation which is used only for this purpose.
    
    Test Plan:
      - Ran upgrade scripts.
      - Verified all users got new secrets.
      - Created a new user.
      - Verified they got a secret.
      - Submitted CSRF'd forms, they worked.
      - Adjusted the CSRF token and submitted CSRF'd forms, verified they don't work.
    
    Reviewers: btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Differential Revision: https://secure.phabricator.com/D8748
    authored
  6. Chad Little

    Better mobile display of ObjectItemView

    Summary: Makes the default 3 rows on mobile devices, with larger fonts. Differential/Audit is much better, Maniphest is maybe a sidegrade depending on setup.
    
    Test Plan: test maniphest, audit, differential list items.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    Subscribers: epriestley, Korvin
    
    Differential Revision: https://secure.phabricator.com/D8741
    authored

Apr 09, 2014

  1. Evan Priestley

    Remove nonfunctional/obsolete 'reconcile.php'

    Summary: Ref T4780. This no longer works and barely ever worked. T4237 is now the relevant task.
    
    Test Plan: Grepped for `reconcile.php`
    
    Reviewers: btrahan
    
    Reviewed By: btrahan
    
    Subscribers: epriestley
    
    Maniphest Tasks: T4780
    
    Differential Revision: https://secure.phabricator.com/D8739
    authored
  2. Evan Priestley

    Disable rate limiting by default in general

    Summary: This is still too rough to enable by default. We can turn it on for `secure.phabricator.com` and tweak it a bit first, and then release it in general with higher defaults and more sensible behavior in edge cases.
    
    Test Plan: Loaded some pages.
    
    Reviewers: btrahan, spicyj
    
    Reviewed By: spicyj
    
    Subscribers: epriestley
    
    Differential Revision: https://secure.phabricator.com/D8736
    authored
  3. Brayden Winterton

    Add a priorityColor property to the maniphest conduit endpoint

    Summary:
    I added a getTaskPriorityColor function to the ManiphestTaskPriority class which returns the color set in the maniphest config for the given priority.
    
    This is in preparation for a change to arcanist which will allow it to display the priority color (if it is a supported color) upon running `arc tasks`.
    
    Fixed some linting issues
    
    Test Plan:
    Invoke the maniphest.info method from conduit and ensure that:
     * The priorityColor property is given in the json
     * the priorityColor property is set correctly
    
    Reviewers: epriestley, #blessed_reviewers
    
    Reviewed By: epriestley, #blessed_reviewers
    
    Subscribers: epriestley, Korvin
    
    Differential Revision: https://secure.phabricator.com/D8734
    authored epriestley committed
  4. Bob Trahan

    OAuth - add a little notes section for admins to remember details abo…

    …ut external accounts
    
    Summary: Fixes T4755. This also includes putting in a note that Google might ToS you to use the Google+ API. Lots of code here as there was some repeated stuff between OAuth1 and OAuth2 so I made a base OAuth with less-base OAuth1 and OAuth2 inheriting from it. The JIRA provider remains an independent mess and didn't get the notes field thing.
    
    Test Plan: looked at providers and read pretty instructions.
    
    Reviewers: epriestley
    
    Reviewed By: epriestley
    
    Subscribers: epriestley, Korvin
    
    Maniphest Tasks: T4755
    
    Differential Revision: https://secure.phabricator.com/D8726
    authored
  5. Evan Priestley

    Make Maniphest project prefill more modern and standard

    Summary: Fixes T4777. We technically support `?projects=...` already, but parse it in an unusual way and apply old, awkward, excessively strict lookups to it.
    
    Test Plan: Used reasonable, standard, human-readable strings to prefill `?projects=` and got the results I expected.
    
    Reviewers: btrahan, chad
    
    Reviewed By: chad
    
    Subscribers: epriestley
    
    Maniphest Tasks: T4777
    
    Differential Revision: https://secure.phabricator.com/D8733
    authored
  6. Brayden Winterton

    Added isClosed property to maniphest conduit endpoint in order to fix…

    … an issue with arcanist when displaying tasks
    
    Summary:
    Arcanist is currently displaying all tasks as closed when invoking `arc tasks`.
    This is because arcanist is setting the display to closed if there is anything in the `status` property. Adding an isClosed property will allow arcanist to properly display open/closed status on tasks by checking against the isClosed property. The isClosed property will be set according to the closed property that is set on each status in maniphest.
    
    Test Plan:
    Invoke the conduit maniphest.info method on any task and insure that:
     # The isClosed property is included in the properties
     # that it is set properly according to the statuses set for maniphest.
    
    Reviewers: #blessed_reviewers, epriestley
    
    Reviewed By: #blessed_reviewers, epriestley
    
    Subscribers: chad, epriestley, Korvin
    
    Maniphest Tasks: T4744
    
    Differential Revision: https://secure.phabricator.com/D8731
    authored epriestley committed
Something went wrong with that request. Please try again.