Re: [RFC] [Discussion] Add get_declared_enums() function

> On Oct 6, 2024, at 3:25 AM, Juliette Reinders Folmer <php-internals_nospam@adviesenzo.nl> > wrote: > > On 4-10-2024 13:44, Nicolas Grekas wrote: > > Hi Nicolas, >> # Introduction of the new function: get_declared_enums() >> >> About this proposal, I shared a one-liner in the previous thread that shows listing only >> enums is trivial already. >> IMHO we don't need this function since the engine already provides everything one >> needs if they want to list enums. I won't object either, I'm just "-0". > > The upside of a PHP native function would be one of performance. > >> # Deprecation of using class_exists() on enum names >> >> This is a big NO on my side. This will break perfectly fine code for the sake of some high >> level ideas that matter less is practice than ensuring stability of the PHP platform. A BC break has >> to be worth it and this is clearly not the case to me. The canonical examples are checks like >> class_exists($c) || interface_exists($c, false) || trait_exists($c, >> false). This is common code to check if a symbol exists in current PHP. >> Yet, with your RFC, all such checks would become broken immediately. > > Well, this statement made me curious to see just _how_ common this type of code is, so > I've done a scan of the Packagist top 2000 projects [1]. > > The check you mention is used a total of 36 times in ~20 packages out of 2000 (includes some > duplication, mostly within Symfony, which also contains the fast majority of the uses of the above > mentioned combination). > > The full break down of the scan results [2] are as follows: > > Combination of the following function calls within a set of parentheses (typically: control > structure condition): > 4 out of 4: class_exists() + interface_exists() + trait_exists() + enum_exists() 3 > 3 out of 4: class_exists() + interface_exists() + trait_exists() 36 > 2 out of 4: class_exists() + interface_exists() 131 > 2 out of 4: class_exists() + trait_exists() 8 > 2 out of 4: class_exists() + enum_exists() 1 > 2 out of 4: interface_exists() + trait_exists() 1 > > > Combination of the following function calls within the same function scope (not necessarily in > the same statement, excluding the above): > 4 out of 4: class_exists() + interface_exists() + trait_exists() + enum_exists() 2 > 3 out of 4: class_exists() + interface_exists() + trait_exists() 17 > 3 out of 4: class_exists() + interface_exists() + enum_exists() 3 > 2 out of 4: class_exists() + interface_exists() 32 > 2 out of 4: class_exists() + enum_exists() 4 > 2 out of 4: class_exists() + trait_exists() 2 > > Please note that not all of this code would need updating. > >> BTW, this makes me wonder if we could have a new symbol_exists() function, that'd >> basically do the above checks in one go? Mixing this idea with Claude's, the signature could be >> symbol_exists($class, $filter = -1, $autoload = true), where $filter is a bitfield that'd allow >> listing only classes, abstract classes, interfaces, enums, traits at will? >> >> # Change of the return value of get_declared_classes() >> >> This proposal is problematic on two aspects: >> The planned BC break feels needless to me. Its motivation is very moot compared to its >> impact on the PHP community, which will be forced to update perfectly fine code. > Again, let's look at the real world impact based on a scan of the Packagist top 2000. > > In the top 2000 packages, there are only 47 calls to the > get_declared_classes() function. These calls are found in 29 > packages. [1][3] > And for the record, there are only 16 calls each to > get_declared_interfaces() and > get_declared_traits(). > > Also note that not all of this code would need updating, only that code which is also targeting > enums. > >> The BC break is planned without any ahead-of-change deprecation notice (except doc of >> course). From a deprecation policy POV, we reject this practice in the Symfony community. We >> don't do "hard BC breaks", or "unannounced" ones: we mandate that any BC >> break is first announced in the current major. This ensures the community won't miss the >> notice, and won't discover the BC break when it's very late and thus costly. There is >> always a way to follow that policy, so I'd strongly recommend adopting this practice in PHP >> itself, and in this RFC today. Here, this could be done by either keeping the function as is in PHP >> 9, or just deprecating it (in favor of get_declared_symbols()?) > > I hear what you are saying and largely agree with you. The problem is, of course, that it seems > unlikely that we can find a way to throw a deprecation for this, as PHP would then also throw a > deprecation for code which needs no changes (which only _intends_ to look at classes, not enums). > > In the RFC, we mention an alternative approach [4], building upon the suggestion by Claude. > This alternative approach would allow for deprecations to be thrown, but would, in my estimation, > need a longer lead-time. Something like: introduce the new function in PHP 8.5, deprecate use of the > old functions in PHP 9.last and remove in PHP 10.0. > > I can imagine combining the alternative approach via get_declared_symbols() with a new > symbol_exists() function like you suggest above (with a similar slow path to deprecate and remove > the old functions). > > On the plus side, the alternative approach makes for much more versatile functionality. In a > number of the cases I looked at, the results from various get_declared_*() functions are combined > before further processing, so having a get-declared_symbols() > function would allow for simplifying that code. The same can be said for the *_exists() functions. > > On the downside, the alternative approach makes for a larger BC break (if combined with > deprecation and eventual removal of the old functions). Given the argued downsides of the current proposal, would introducing get_declared_symbols() + symbols_exists() really require deprecating the other functions? It seems adding a get_current_enums() and get_declared_symbols() + symbols_exists() functions would address all the same userland developer needs as the proposed RFC (and more) with the only caveat being get_current_classes() remains confusing. To address that one remaining concern why not use add a highly visible recommendation at the top of the get_current_classes() docs page informing people to use get_declared_symbols() when they need only classes. Is there a reason this approach would not be the best way forward compared to the other options currently being discussed? -Mike P.S. And if it is deemed critically important to get rid of the potential confusion regarding the existing get_current_classes() behavior it could be "ultrasoft" deprecated, meaning removed in PHP 11, or just left in forever with a warning (although for the life of me I still do not understand why some people are against leaving a deprecated function in forever, especially for things that are changed for clarity, not because they cause harm by themselves.)