Page MenuHomePhabricator

Correct a datasource issue when viewing repository URIs in "Manage Repository"
ClosedPublic

Authored by epriestley on Jun 30 2017, 2:01 PM.
Tags
None
Referenced Files
F13370335: D18176.id43726.diff
Thu, Jun 27, 5:04 PM
F13366320: D18176.id43724.diff
Wed, Jun 26, 7:25 PM
F13353329: D18176.diff
Sun, Jun 23, 8:25 PM
F13325945: D18176.diff
Sat, Jun 15, 2:25 AM
F13304832: D18176.id43726.diff
Sat, Jun 8, 1:24 PM
F13300226: D18176.id43724.diff
Fri, Jun 7, 8:58 AM
F13294880: D18176.id.diff
Wed, Jun 5, 8:33 PM
F13293846: D18176.diff
Wed, Jun 5, 1:56 PM
Subscribers

Details

Summary

Fixes T12884. In cases other than this UI, applications access URIs through the Repository they're part of. This means that applications interact with URIs which have gone through the correction/adjustment logic in PhabricatorRepository->attachURIs(), which fixes up "builtin" URIs to have the right values based on configuration.

In this case (and, as far as I can tell, only this case) we load the URI directly and act on its properties which depend on configuration and repository state.

This can mean we're using a different view of the URI than we should be.

To fix this: after loading the URI, reload it through the repository so the relevant adjustments are applied.

I think this is the most reasonable fix. We could try to make RepositoryURIQuery somehow enforce this, but the cost of this error is small (mild confusion about display state), the other things which do direct loads don't depend on this state (editing), and everything else loads via a repository and is likely to continue doing that forever.

Test Plan

Screen Shot 2017-06-30 at 6.56.03 AM.png (849×1 px, 134 KB)

Diff Detail

Repository
rP Phabricator
Branch
uri1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 17612
Build 23640: Run Core Tests
Build 23639: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Jun 30 2017, 2:05 PM
This revision was automatically updated to reflect the committed changes.
jmeador added inline comments.
src/applications/diffusion/controller/DiffusionRepositoryURIViewController.php
30

@epriestley I believe this query is resulting in 10 second page loads for us. I haven't tested it, though so I can't be sure. It does look like it is loading every URI for every repository though. Is this intentional? would a constraint on this query break how this fix works?

Yikes! Pretty sure the query is just completely wrong, give me a second and I'll get a fix out.

D19036 should make the cost of this page more reasonable.