Differential D20659 Inline Comments

Modularize auth provider configuration
Back to Revision

Inline Comments

Diff	Status	Reviewer	Comment	Created
Current		amckinley	Maybe this should also be moved to `PhabricatorAuthConfigPropertyTransaction`?	370 w
Current		epriestley	Moving this makes sense to me.	370 w
Current		amckinley	This is also new.	370 w
Current		epriestley	Ideally, we would raise this error only if (a) the transaction actually attempts to change the value and (b) probably, the new value is "enabled". See D20311 for a somewhat-recent somewhat-relevant example. This is mostly for API callers, but the ideas here are: - It's okay to write a client that shows the user a form, then submits all the transactions that "Save" implies. We don't require the client to know whether each field is editable or valid: it's okay for them to send back the full current state of the object and let us validate it. - A specific subcase of this is when a task has an invalid value for some `<select />` field, like priority set to "zebra". It's okay to apply a transaction which changes the priority from "zebra" to "zebra", even though "zebra" is not valid, because if we don't allow this it's super tough to write a sensible form on the client that doesn't get in the user's way in the face of questionable data. - `validateTransactions()` is doing (at least) two types of checks: one type of checks is structural ("is this value in a plausible format?") and one is more permissions-ey ("is the value itself a legal value which the user has permission to select?"). We could structure the code like this instead: ``` validateTransactionStructure($xactions); $xactions = filterNoOpTransactions($xactions); validateTransactionValues($xactions); ``` ...but I think most of the time these checks are pretty part-and-parcel and this would lead to a lot of duplicate code, even though it //might// technically be more clear. An adjacent issue is that you can submit transactions (for an object with title "B") like "set title to A" and also "set title to B" in the same transaction list. These merge+filter into a no-op (since we change the title, but then change it back), but in this case we //do// want to raise an error. Basically, there are a million weird cases here and I think we're structurally in reasonable territory where badness is at least relatively low, if not actually minimized, but one piece of badness is that you unintuitively should allow not-exactly-valid-but-no-op transactions to proceed through `validateTransactions()`.	370 w