href can be an object - PhutilUri, and maybe some other things.
what is this method for anyway? ie what usecases is a PhuiTag not know what kind of tag it is?

In D25153#4709, @avivey wrote:

href can be an object - PhutilUri, and maybe some other things.
what is this method for anyway? ie what usecases is a PhuiTag not know what kind of tag it is?

It seems a nice case for phutil_nonempty_stringlike() that - on objects - it calls the internal __toString()

But, it would be nice to add an inline PHPDoc for the href attribute to mention that thing like

/**
 * Destination URI
 * @var string|PhutilUri
 */
private $href;

I'd like to know more about why this method even exists, and what other things might end up in href.

maybe add call to phlog() if it's of an unexpected type.

In D25153#4715, @avivey wrote:

I'd like to know more about why this method even exists, and what other things might end up in href.

The thing is, before this change, strlen() was casting everything to string under the hood (so, objects were fine, if the developer implemented a __toString()).

After this change:

• phutil_nonempty_string(): it requires to be null or a string, and not "" but does not accept any object - so it's not OK here
• phutil_nonempty_stringlike(): it also accepts objects but only if you can cast them to a string (→ if the developer implemented a __toString()) and only if the result is not "" - so it should be 99.99% OK here

With this premise I'm 99% sure that phutil_nonempty_stringlike() is just what fits here. No need to phlog().

The phutil_nonempty_stringlike() already explodes by design if an object without a good __toString() is passed, creating a log line.

a phlog would be useful, because anything other than string or PhutilUri would be very surprising, and possibly a bug. My desire is to expose bugs as much as possible, not hide them.

In D25153#4717, @avivey wrote:

a phlog would be useful, because anything other than string or PhutilUri would be very surprising, and possibly a bug. My desire is to expose bugs as much as possible, not hide them.

I understand but in these cases the bug emerges because we can see the value of _toString() exactly as before.

I don't think Evan would put a phlog() in this position. But I don't have a strong opinion.

I'd argue that almost any use of phutil_nonempty_stringlike() is hiding a bug, because it hides an object we don't know the type of - which is by itself a problem.

In D25153#4720, @avivey wrote:

I'd argue that almost any use of phutil_nonempty_stringlike() is hiding a bug, because it hides an object we don't know the type of - which is by itself a problem.

Yep I agree since that was also obviously true with strlen().

So if you want to proceed with a stricter check, I can suggest to implement that in the href setter method (to slow down once), and not in the getter (to slow down every read call)

putting it in the setter is fine...

With much surprise, even if visiting 100+ pages and logging anything that is not null or a string, I was NOT able to log any object.

Hoping to be useful I propose a general logger in the setter without even checking for a particular instanceof something. And, I think we can keep the phutil_nonempty_string() and just fix on any incoming report.