On Fri, Jun 12, 2020 at 6:32 AM Troy McCabe <troy.mcc...@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
