Remove variable definition now unneeded outside of the if clause

Here's a revision which does not throw a red box via ->setFormErrors() at the user but at least it is displaying the Required text and turning it into bold red after clicking Award without having defined a recipient. So the line $errors[] = pht('Badge name is required.'); is currently never ever shown.

Thanks for the hints how to improve this! Appreciated.

That was a merged JS change so I should have also run ./bin/celerity map. Sorry. Followup patch in https://we.phorge.it/D25637

I don't believe in playing whack-a-mole on "could this be a password" but a use case I've been recently thinking of is "Do not allow task creation when task content/data is exactly the defaults provided by the Form used to create the task". Basically: You were supposed to fill in some stuff but you did not when creating your task.

Uhm, seems I was somehow off by one line. I thought that this comes from escapeToken($repository->getCallsign()) being optional nowadays but seems to be triggered by escapeToken($repository->getRepositorySlug()) instead which indeed would be surprising. This might be a patch to abandon because might hide an underlying issue.

Garrr I am so sure I had tested it this morning but maybe I was sleepwalking? Tested again now and seems to work, thanks

Giving a +1 as I had tested this in D25080#16527 but I would prefer to see the two strings adjusted in D25080#16549

I currently cannot think of other potential side effects (mail notifications about task creation seem to have had a [Created] prefix taking priority over other actions for years, and web notifications in the top bar also still behave as before), so I'll give my +1

The coffee on my desk doesn't seem to work yet - use a proper comparison.

Use standard JS string concatenation instead of shiny padStart() because better backwards compatibility

In D25630#17685, @valerio.bozzolan wrote:

I think somebody here would propose to avoid padStart() and be boring:

if (d_m.length < 2) {
  d_m = '0' + d_m;
}

In T15820#17365, @valerio.bozzolan wrote:

Just because we can, right? Or any real concern?

The approach taken in D25621 is more wholesome and preferable thus abandoning this

This makes sense.

Tested locally before and after applying this branch, with two users sending messages in a chat room after clicking the speechbubbles icon in top bar and selecting "Persistent Chat", both functionality wise (clicking the bar in the lower right corner) and by inspecting the element in the web browser's developer tools' inspector. Works as expected.

Thanks for working on this! Works pretty well locally. From a quick test of this branch, while being logged out,

• Maniphest Advanced Search: Setting Assigned to to Current Viewer, I get a Login page.
• Diffusion Commits Advanced Search: Setting Authors to Current Viewer, I get a Login page.
• Differential Advanced Search: Setting both Authors and Reviewers to Current Viewer, I get a Login page.
• Differential Advanced Search: Setting Responsible Users to Current Viewer, I get an exception (DifferentialResponsibleDatasource - probably another source to cover here?)

Should somebody file a ticket for that?

I'm purely setting "Request Changes" to reflect that IMHO this should not get merged until there is an approach to fix T15579 - otherwise in some corner cases this proposed change would create broken URIs.

I'd prefer not to expose this as an always-visible button simply because it takes screen estate on smaller screens.
Maybe an entry Copy Revision Permalink between two separators in the expandable "View Options" dropdown is an option?

I can confirm this works as expected for the task view itself in e.g. http://phorge.localhost/T1168 . This seems to also fix http://phorge.localhost/feed/ so it does not only show me about setting high priority on a new task, but instead shows that the task was created? That would be a nice side effect too IMO.

Tested this patch on http://phorge.localhost/feed/transactions/ and in the feed at the bottom of http://phorge.localhost/W20 itself.
Also tested changing panel visibility from Public to Admin only and accessing these URIs as an average user.
All working as expected, no explosions.

In T15815#17250, @valerio.bozzolan wrote:

being able to have people running unit tests and see the results in their language was a feature

I tested this one-liner as part of the downstream conversation that pppery and I had in https://phabricator.wikimedia.org/T363364, still I would ideally prefer another upstream opinion before accepting this revision as O1.

In D25608#17388, @avivey wrote:

This is probably the right fix (or maybe moving shouldAllowPublic() to the base PhabricatorFactController).

(Per my last comment)

I guess that would imply editing 15-20 *ConduitAPIMethod.php files which currently

$limit = $request->getValue('limit');
if (!$limit) {
  $limit = $this->getDefaultLimit();
}

and add something like

if (!is_int($limit) && !ctype_digit($limit)) {
  throw new Exception(pht('Field "limit" must be an integer value.'));
}

? Hmm.

That's because we applied D25415

Add return value (though it seems to make no difference)