Everything

php

Home » Project » Newsgroups » php.internals » Make Reflection*::getDocComment() return an empty string instead of false »

Make Reflection*::getDocComment() return an empty string instead of false

php.internals

Gina P. Banyard

1 year ago

Hello internals,

While working on the deprecation to/from bool type juggling in functions RFC and seeing the impact within Symfony, we found a common slightly annoying case.
The getDocComment method of various Reflection classes was always used as a string, and we thought changing the behaviour in PHP made more sense.

I submitted a PR [1] but was asked to gather feedback on the mailing list.

Please let me know what you think.

Best regards,

Gina P. Banyard

[1] https://github.com/php/php-src/pull/18928

Christian Schneider

1 year ago

Am 25.06.2025 um 12:37 schrieb Gina P. Banyard <internals@gpb.moe>:

> While working on the deprecation to/from bool type juggling in functions RFC and seeing the impact within Symfony, we found a common slightly annoying case.
> The getDocComment method of various Reflection classes was always used as a string, and we thought changing the behaviour in PHP made more sense.
>
> I submitted a PR [1] but was asked to gather feedback on the mailing list.
>
> Please let me know what you think.

In general I like out-of-band instead of in-band signalling as it seems more robust to me.

I am a bit confused why it was false in the first place and not null as the absence of something was in my book the definition of null.
But then again I realise there is a general aversion against null.

I noticed that the "empty string for absence of value" was also used in DOMElement::getAttribute() but you can argue that the existence of DOMElement::hasAttribute() fills this gap.

Anyway, I'm not a big fan of this move but won't fight it either.

Regards,
- Chris

Andreas Hennings

1 year ago

On Wed, 25 Jun 2025 at 13:56, Christian Schneider <cschneid@cschneid.com> wrote:

>
> Am 25.06.2025 um 12:37 schrieb Gina P. Banyard <internals@gpb.moe>:
> > While working on the deprecation to/from bool type juggling in functions RFC and seeing the impact within Symfony, we found a common slightly annoying case.
> > The getDocComment method of various Reflection classes was always used as a string, and we thought changing the behaviour in PHP made more sense.
> >
> > I submitted a PR [1] but was asked to gather feedback on the mailing list.
> >
> > Please let me know what you think.
>
> In general I like out-of-band instead of in-band signalling as it seems more robust to me.

I agree with this.
I was not familiar with the terms, but they make a lot of sense.
This said, I have experienced that some people feel different, and
want to return '' all the time..

In general, "return null" becomes more valuable if an empty string
would have a different meaning.
In this case, an existing doc comment can never be an empty string. So
there is no ambiguity if we would use ''.
But semantically, I think of "no doc comment" as different from "empty
doc comment", and null (or false) reflects that better than an empty
string.

>
> I am a bit confused why it was false in the first place and not null as the absence of something was in my book the definition of null.
> But then again I realise there is a general aversion against null.

I think false is just "legacy null" for a lot of php built-in
functions and methods.
Any argument for the aversion to null would equally apply to false,
when used as a "not found" value.

If starting from scratch, I would prefer "return null" in all of those
places. But at this point I don't think the BC break is worth it, so
the "return false" will follow us around for a long time.

(The BC break applies to extending and to conditonal checks with === false)

>
> I noticed that the "empty string for absence of value" was also used in DOMElement::getAttribute() but you can argue that the existence of DOMElement::hasAttribute() fills this gap.

I think all this shows is that different parts of php were created by
different people, or that somebody changed their mind over time.
To me, "return null" would be preferable for
DOMElement::getAttribute(). https://3v4l.org/QHXlA

Cheers
Andreas

Rowan Tommins [IMSoP]

1 year ago

On 25 June 2025 13:21:22 BST, Andreas Hennings <andreas@dqxtech.net> wrote:

>On Wed, 25 Jun 2025 at 13:56, Christian Schneider <cschneid@cschneid.com> wrote:
>>
>> I noticed that the "empty string for absence of value" was also used in DOMElement::getAttribute() but you can argue that the existence of DOMElement::hasAttribute() fills this gap.
>
>I think all this shows is that different parts of php were created by
>different people, or that somebody changed their mind over time.
>To me, "return null" would be preferable for
>DOMElement::getAttribute(). https://3v4l.org/QHXlA

In that example, PHP shouldn't be deciding its own behaviour at all, it should be following the DOM  standard.

It turns out, the specification explicitly states that the return value should be null in this case, so arguably it's a bug that PHP's implementation does not: https://dom.spec.whatwg.org/#ref-for-dom-element-getattribute%E2%91%A0

Rowan Tommins
[IMSoP]

David CARLIER

1 year ago

Hi,

It makes more sense to me too to change this behavior. I would rather go
with null though ; but that is just personal preference ; empty string is
already better than false ... Did you assess how widely it is used before
the users come back "angry" ?

Cheers.

On Wed, 25 Jun 2025 at 11:39, Gina P. Banyard <internals@gpb.moe> wrote:

Rowan Tommins [IMSoP]

1 year ago

On 25 June 2025 11:37:37 BST, "Gina P. Banyard" <internals@gpb.moe> wrote:

>The getDocComment method of various Reflection classes was always used as a string, and we thought changing the behaviour in PHP made more sense.

Looking at the stub file, the use of false instead of null to signal "no value" seems to be used in a bunch of places in that extension. For instance, getExtensionName() apparently returns false to indicate a user-defined function, while getFileName() returns false for the opposite case.

Returning an empty string when there's actually no comment feels a bit awkward and inconsistent - for example, we don't return a "no type" object from getReturnType().

I think returning null rather than false for all of these would be preferable. From a compatibility point of view, it's not much different from changing to return '' or 0 for those cases, but feels more logical as a design.

Rowan Tommins
[IMSoP]

Kamil Tekiela

1 year ago

What exactly is the context in which symfony uses it?

Alexandru Pătrănescu

1 year ago

On Wed, Jun 25, 2025 at 4:22 PM Kamil Tekiela <tekiela246@gmail.com> wrote:

> What exactly is the context in which symfony uses it?
>

Was wondering just that, and I can only imagine it's a function call that
receives the parameter as a string and it would  getDocComment() result is
passed directly, sometimes being false.
Example:
https://github.com/symfony/symfony/blob/a3c1d1f9e9bbac9933cc3792a55e756eca5bb495/src/Symfony/Component/DependencyInjection/ContainerBuilder.php#L1177

I think that for those cases `getDocComment() :? ''` might be a faster fix
and move on with it.
And symfony already does this in some places:
https://github.com/symfony/symfony/blob/a3c1d1f9e9bbac9933cc3792a55e756eca5bb495/src/Symfony/Component/DependencyInjection/Compiler/AutowireRequiredMethodsPass.php#L53

IMHO, it's not worth the compatibility breakage, as some others might
already compare it using `=== false`.

--
Alex

Larry Garfield

1 year ago

On Wed, Jun 25, 2025, at 5:37 AM, Gina P. Banyard wrote:

> Hello internals,
>
> While working on the deprecation to/from bool type juggling in
> functions RFC and seeing the impact within Symfony, we found a common
> slightly annoying case.
> The getDocComment method of various Reflection classes was always used
> as a string, and we thought changing the behaviour in PHP made more
> sense.
>
> I submitted a PR [1] but was asked to gather feedback on the mailing list.
>
> Please let me know what you think.
>
> Best regards,
>
> Gina P. Banyard
>
> [1] https://github.com/php/php-src/pull/18928

The use of false as a sentinel value is always wrong[1], so I agree with the issue.

Whether the BC break is worth it is a harder question.

Whether it changes to null or empty string, I think, depends on whether in context empty string is a reasonable "zero value."  Viz, will it fail gracefully automatically, the way an empty array would for "get list of things"?  If so, empty string is probably better.  If not, null is reasonable given the current state of the language.  Null is arguably more purely correct, but empty string may be more ergonomic in practice.  I defer to Nicolas who has probably had to deal with it more than I ever will. :-)

[1] https://www.garfieldtech.com/blog/empty-return-values

--Larry Garfield