Update as per peer review

Yeah - spotted & fixed!

PHP != Perl

Thanks for the fix! <3

• test with object

• flip condition

In D25326#9471, @avivey wrote:

Maybe we can pass also an object that has a __toString() method

do you have an example on how to construct one?

Yeah no problem, I just wanted to understand and share each other points.

In D25326#9469, @valerio.bozzolan wrote:

Maybe we can pass also an object that has a __toString() method

Explicit is better then implicit.

Maybe we can pass also an object that has a __toString() method

Using $value === '' would be only place in the code that does that

In D25325#9466, @valerio.bozzolan wrote:

Yeah but I don't see that readability bonus point here since we have a "not non-empty" condition.

Yeah but I don't see that readability bonus point here since we have a "not non-empty" condition.

In D25325#9464, @valerio.bozzolan wrote:

I mean, so the goal of the cast is to be able to operate strictly with strings, so finally we can if($v === '') without any pitfall

I mean, so the goal of the cast is to be able to operate strictly with strings, so finally we can if($v === '') without any pitfall

Note that after phutil_string_cast(), NULL is never returned, but just a string

Awesome, tyty

• More test cases

Nice. Could we add some additional tests for Boolean true/false, the number zero, populated and empty array?

(looks like most other custom fields already override this method with something useful).

I agree that non-string/null should be handled differently. I guess I don’t see the difference between null + strlen being used vs. the proposed nonempty_string/stringlike, and that making that change is explicitly acknowledging that casting is expected/intentional when it isn’t and instead the different types should be handled appropriately (your suggested long-term solution).

I'm pretty sure that any place where strlen is getting something that isn't a string (including int), that's actually a bug. In Phabricator code, strlen was only supposed to be used when expecting a string or null; All the places we now find aren't strings, are places where we were wrong about the type of the object.
Epriestley says something like that at https://secure.phabricator.com/T13588#257329 .

phabricator_form() is called in 24 places, and 23 of those specify the method.

So the choices are:

1. Update the one calling place which isn't specifying the method, but leave phabricator_form() as a fragile function.
2. Update phabricator_form() to defensively handle the lack of a method attribute, as we have done here.
3. Update the phabricator_form function signature to make the method a required parameter.

Honestly I do not love the idea of if(strlen($something)) to answer the question "Is this empty when casted to string?".

I've added both links to the preamble for easy discovery.

Wouldn’t this be better as a null + strlen check? It was originally a strlen I assume.

Good content but unfortunately undiscoverable if not linked from a form preamble

• Feature request: https://we.phorge.it/book/contrib/article/feature_requests/
• Bug report: https://we.phorge.it/book/contrib/article/bug_reports/

Remove already implemented text parts

1. This is a feature request, it should be defined using Feature Request pattern, not Bug Report pattern.

1. This is a feature request, it should be defined using Feature Request pattern, not Bug Report pattern.
2. We already implemented this in D25035

I'd say "This account has been disabled" is way clearer than... nothing? :P

Thanks :) What do you think about just returning a null User description? I propose that since the User profile view already says "Disabled".

I don't think I intended to land this change.

Looks like a mid-air collision, heh :)

Looks like a mid-air collision, heh :)

In D25320#9382, @valerio.bozzolan wrote:

update celerity

update celerity