Haha to be clear we're using an open-source self-hosted tracking solution, I was just throwing GA out there as an example. But I do understand your point vis-a-vis data security.

Why do you want to track users with Google Analytics or a similar library?

haha nice. I just posted this comment on our downstream task tracking this.

I couldn't find a project with multiple subprojects on this install otherwise I would have linked you to an example

👍 testing it now.

We've run into this a couple of times. When a user updates a revision through the web UI, the repository field (at /differential/revision/update/{REVID}/) doesn't auto-populate with the revision's repository and people sometimes forget to fill it out. This causes the repository to be stripped from the revision as well.

In exchange, here is a cat on a mat trying to do some yoga. Not my cat, but she was at an airbnb I stayed at and my decision to not steal her came down to a coin toss

that's a great pup. 10/10

send puppy pics please

verified

Just to clarify, I tested it locally, not on this install.

Just spent some time tracking down what ended up being this issue. I wonder if adding a check in this block to query for the given revision would be sufficient enough to detect this case and return a more sensible error message. I can submit a diff for that if that'd be acceptable

ah yep that seemed to work. I was still specifying releaseNotes for the field which I'm guessing is why it wasn't getting saved. Thanks again for all the help! If you're ever in Pittsburgh I think I owe you about 50 beers at this point.

I pulled in that change and I'm still seeing the same issue. Editing the field from /differential/revision/edit/... seems to work normally, but adding it via the differential.createrevision endpoint does not.

Checking in for guidance in case this is the wrong way to go. I have a custom field which extends DifferentialStoredCustomField and I'm trying to convert it in the same way as the above. Thus far my field isn't being saved when calling differential.createrevision. I've included the DifferentialCommitMessageCustomField and the DifferentialStoredCustomField below.

Thanks that did it! Still working through an issue with one of our more *ahem* exotic applications, but I suspect that'll be a more involved refactor on our end.

Doing that without defining getFieldTransactions() in my class yields this exception:

Transaction with key "1" has invalid type "devTestPHIDs". This type is not recognized. Valid types are: update, comment, title, summary, testPlan, reviewers.add, reviewers.remove, reviewers.set, repositoryPHID, tasks.add, tasks.remove, tasks.set, view, edit, projects.add, projects.remove, projects.set, subscribers.add, subscribers.remove, subscribers.set, phabricator:auditors.

Thanks for the help Evan. I've gone through and converted everything as you suggested. The conduit method then started telling me

Confirmed this on my local. It seems to be due to D16851, specifically this line: https://secure.phabricator.com/source/phabricator/browse/master/src/applications/diffusion/application/PhabricatorDiffusionApplication.php$104.

Pretty sure this works as intended

{auto-playing-audio WilhelmScream.mp3}

Yes I did have the persistent chat pane open. Also can't seem to repro without it.

The other errors that appear in DarkConsole:

Hmmm I'm not seeing those lines in that file anywhere. On the current head of master, I'm pretty sure the viewer is always set to PhabricatorUser::getOmnipotentUser() in TransactionPublishWorker. Although maybe that isn't making it all the way to the policy aware query in every case.

@jessjohnson I still haven't had a chance to dig further into this, but I'll report back when I do (or if you figure anything out, please let me know). I don't think killing off the tasks permanently is really an option since this seems to happen for specific degenerate instances of otherwise-needed tasks. Perhaps not allowing these tasks to retry would be an okay bandaid, but I'd still like to figure out the underlying problem if we can.

Nope we don't have that integrated in yet (just missed our last cutoff). Once we get that pulled in I'll confirm it clears up the issue.

Evan, do you think there is a route forward with this that would be feasible for me to implement upstream?

Ah yeah it could be related to that (policy errors/unit test failures), although I wouldn't really describe this as "extremely rare" (except of course when I'm trying to make it happen). Some stats from grepping through logs: since last week it looks like this error has happened 73k times involving 193 distinct diffs. It's like the daemons are laughing at my inability to repro it.

Just spent a bit of time trying to repro this in a script but could not. The diffs always load successfully from the script. I even tried waiting until right after one of the errors popped up in the logs and then queried for that diff via the script right away.

I've got the endpoint and controller set up, but I get the following error when I call PhabricatorFile::newFromFileDownload:

Alright I've been able to narrow this down a little bit more. There's some error with how external/pear-figlet/Text/Figlet.php is smushing characters. Specifically lines 339-362 as well as the _rep($r) function. The equivalent function in the C code is here. I haven't been able to find an obvious discrepancy, but I suspect there's a subtle difference in how one of the regex functions is handling things or an error in one of the regexes themselves. This error also occurs with backticks and double-quotes: