> On Oct 6, 2024, at 3:25 AM, Juliette Reinders Folmer > <php-internals_nos...@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.)
