Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:110499
MIME-Version: 1.0
References: <CAD2aB0df7hObz8=AOGHJKAR3Jc2tdAU=y0i-oR_XfqHo-rG1fQ@mail.gmail.com>
 <CAF+90c8_8N5xKe3EGms1i_TbB7v3vPVSjr0tYapiZyxU+0diFA@mail.gmail.com> <CAD2aB0eNcSLHp+-XPLySfQxb08E6xfcC9htAA3C_M3Qyz15OpA@mail.gmail.com>
In-Reply-To: <CAD2aB0eNcSLHp+-XPLySfQxb08E6xfcC9htAA3C_M3Qyz15OpA@mail.gmail.com>
Date: Fri, 12 Jun 2020 16:10:41 +0200
Message-ID: <CAF+90c_mKOtyBg_whG7FM2i3jTtofcCWtPvHRqtqh=cCV_GHJQ@mail.gmail.com>
To: Troy McCabe <troy.mccabe@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000069b6f305a7e3a545"
Subject: Re: [PHP-DEV] Revisiting proposal for addition of `get_class_constants()`
From: nikita.ppv@gmail.com (Nikita Popov)

--00000000000069b6f305a7e3a545
Content-Type: text/plain; charset="UTF-8"

On Fri, Jun 12, 2020 at 6:32 AM Troy McCabe <troy.mccabe@gmail.com> wrote:

> Hey Nikita,
>
> Thanks for the thoughts.
>
> > Could you please explain in more detail *why* we should duplicate
> existing reflection functionality into free-standing functions?
>
> In terms of the *why*, there were three main reasons:
> 1. It aligns with the addition of the functions referenced in the
> original post (`str_[contains|starts_with|ends_with]()`), and one
> their stated reasons of simplifying the API for userland developers.
> While `(new \ReflectionClass(MyClass::class))->getConstants()` isn't
> the most difficult thing to grasp, it's not immediately clear to new
> developers, and is more verbose than
> `get_class_constants(MyClass::class)`
>

I don't think this comparison makes a lot of sense. str_contains() etc were
very much about providing a more convenient way to perform an extremely
common operation. It makes sense to make common operations more concise.

get_class_constants() is on the complete other side of the spectrum: It
would be a very niche API. It's okay to have niche APIs (Reflection as a
whole is pretty niche), but there's no point in optimizing them for concise
calls.

2. `get_class_[methods|vars]()` existing as built-in functions,
> creates a gap to retrieving class constants in the same way. If I
> start down the path of class inspection using `get_class_*()`, but
> find I can't retrieve constants in the same way, this is an
> inconsistency.
>

That's correct, but I think it's acceptable as long as we consider those to
be historical artifacts.


> 3. When using Reflection, accessibility is not respected as it is with
> the `get_class` family of functions. In the event that a developer is
> looking for constants which are accessible to the current context,
> there's no way (that I'm seeing, anyway) to retrieve _only_ constants
> accessible in the current context.
>

This is a good point. I think the right way to address this would be to
expand the reflection API though. It's a bit odd that visibility-based
filtering exists here, but is not available in the reflection extension (it
can filter by public/protected/private, but not by "visible in this
scope"). Having methods like ReflectionMethod::isAccessible(?string $scope
= null): bool would be a good addition, I think.


> > I believe the existence of functions like get_class_methods() is a
> historical artifact, because they were introduced before Reflection was a
> thing. Unless there is a strong reason to the contrary, I would prefer
> reflection functionality to stay inside Reflection...
>
> This is good background that I wasn't aware of (I knew the Reflection
> API was newer than the built-in functions, but not that the
> `get_class_*` functions were generally frowned upon).
>
> It does bring up 2 questions:
> 1. Obviously this is a much larger discussion, but is there any
> appetite to deprecate & remove the existing functions in favor of the
> Reflection API?
>

I don't think there's a particularly strong reason to remove those
functions. They're there and they don't hurt anyone :) I think there's a
middle ground between "these must be removed" and "these must be extended
further".


> 2. An alternative to adding `get_class_constants()` would be to
> introduce `ReflectionConstant` as a return type from
> `ReflectionClass::getConstants` to match `ReflectionMethod` &
> `ReflectionProperty`, which would solve point 3 above. Would this be a
> preferable approach?
>

As you mention in your next mail, this already exists :)


> > You do mention performance as a benefit, but it's not immediately
> obvious to me which use-cases are bottlenecked by class constant reflection.
>
> Enum implementations are the big case for this. While the libs I've
> looked at use an internal cache, these caches are per-request, so
> reflection will need to be used as many times as there are enums in a
> given system. Depending on the scale, this could be an appreciable
> amount. Obviously external caches could be leveraged, but that then
> requires additional development lift, instead of using battle-tested
> enum libs.
>

I think the per-request cache here is the right approach. Reflection isn't
so slow that hitting it once per request would be problematic. At the same
time, no matter how this is implemented, you will still want to have that
cache, because fetching the class constants is still going to be slower
than not fetching them, regardless of how you do it. Having this as a free
function may be two times faster than doing it via reflection, but not
doing it at all is #INF times faster :)

Regards,
Nikita

--00000000000069b6f305a7e3a545--