Should there be a `get_declared_enums()` function ? View as Markdown

I'm presuming it's too late for PHP 8.4, what with feature freeze having come & gone this week.

Based on the mostly supportive responses on the list, I wonder whether an RFC is needed. If so, I'd be happy to create an initial draft (for PHP 8.5).

Thank you!

We now have the Release Cycle Update RFC approved
(https://wiki.php.net/rfc/release_cycle_update#allow_features_that_do_not_require_an_rfc_in_the_beta_period),
and it does allow minor new features during the beta period.

Quoting from the RFC:

[snip]
It therefore seems illogical to allow RFC-requiring features to be merged during the beta period, but not minor improvements that do not require an RFC, such as adding constant, parameters, config options, or even small functions or methods.

We propose to explicitly allow minor features, optimizations, and internal API / ABI changes, during the beta period.

I think the get_declared_enums is small enough to be considered a
minor change given we don't change any engine internals. Given the
positive response on the mailing list, I think we can have this in PHP
8.4 as well, subject to RMs approval of course. There's also a PR from
yesterday (https://github.com/php/php-src/pull/15446) that added a new
Curl constant.

I went ahead and created PR https://github.com/php/php-src/pull/15443 along with tests, UPGRADE notice, etc. I think having a get_declared_enums function will be helpful. The implementation is simple and straightforward too.
Thanks for creating the PR Ayesh!

I'm presuming it's too late for PHP 8.4, what with feature freeze
having come & gone this week.

Based on the mostly supportive responses on the list, I wonder whether
an RFC is needed. If so, I'd be happy to create an initial draft (for
PHP 8.5).

Smile,
Juliette

I would prefer to have an RFC for this, even if it's short. I'm not against it, I just think there's enough non-trivial questions (eg, impact on get_declared_classes()) that it warrants an RFC process/discussion.

--Larry Garfield

On 16.08.2024 at 17:11, Nicolas

And here is a one liner:

function get_declared_enums() {
     return array_filter(get_declared_classes(), 'enum_exists');
}

Nice!

Are you suggesting that there is no need for get_declared_enums() in the
core, since it can be implemented in userland with a single function call?

I think he just compulsively golfs inefficient PHP code (I have the same
affliction).

Cheers,
Bilge

Hi Juliette,

To everyone on this thread and thus nobody in specific (hence my top post):

Given that get_declared_*() populates a new ZEND_HASH each time the function is called[1] — which for classes can easily be hundreds of strings — would there be any appetite to:

1. Add an optional parameter — e.g. get_declared_classes($startingFrom) — that can limit the number of symbols starting with "n" where if n==100 then it would return array_slice($symbols,100), and

2. Add a function — get_declared_class_count() — that returns the current number of declared symbols of whichever kind of symbol, so we can get the "n" before calling include/require to use with get_declared_*() after calling include/require?

This would cut down on creating hashes with 100+ strings that get immediately thrown away, and also cut down on memory fragmentation/garbage collector churn.

-Mike

[1]
https://github.com/php/php-src/blob/18d41502da0da1bb3928e60c41f1b821974c2c01/Zend/zend_builtin_functions.c#L1371

Mike,

While your suggestion does not directly relate to what triggered my mail to the list, I very very much like the idea.

Excellent!

It's as if you are reading my mind or rather, reading one of the autoloaders [1] I need to maintain, in which your above outlined proposal should definitely make for more efficiency.

Thank you for sharing that. Yes, your use-case is very similar to mine.

Having said that, the same parameter + extra function would then also be needed for get_declared_interfaces(), get_declared_traits() - and, if it would be added, get_declared_enums().

That was my assumption and intention, but I did not say so explicitly. I should have.

Thanks to Ayesh's excellent PR, I was able to recognize that all of those functions in-fact are implemented via one internal function get_declared_class_impl() so it only made sense — to me, at least — that modifying it would enable the same functionality for all four (4) userland functions.

There is one problem I can see with this approach though: since PHP 7.4, the return value of get_declared_classes() (and friends) does not guarantee any particular order anymore [2].
So, an array_slice($symbols, $n) may not get you the latest classes loaded, so I think this would only work if the order of classes is guaranteed in some way.

That is a super relevant insight and one I was actually not aware of.

Do you happen to know if it was triggered by an explicit RFC to that effect, or if it resulted because of a side-effect of some other implementation change? From my perspective, that is definitely a BC break and one that I am surprised I never noticed before? Do you also know if it is a case of "not guaranteed, but in-fact actual fact it always works that way, or not?"

I have not used my lib that needs that functionality to work in a few years, so maybe it is broken now and those who are now maintaining those apps have had to deal with it?

Given that you see value in the same use-case I had which was an autoloader, maybe it would be better to achieve the functionality we need in a different, more efficient way?

What if instead PHP were to implement an optional 2nd callback parameter to include() / require() / include_once() / require_once() to allow us to capture the symbols loaded and their paths? The callback function could return void and accept an array of $symbols with the following guaranteed minimum structure?

$symbols = array(
'classes' => [],
'interfaces' => [],
'traits' => [],
'enums' => [],
);

With that you could reimplement all that PHP code you linked — at least for versions of PHP supporting the callback — to look like this:

function loadFile($path)
{
if (strpos(DIR, 'phar://') !== 0) {
$path = realpath($path);
if ($path === false) {
return false;
}
}
if (isset(self::$loadedClasses[$path]) === true) {
return self::$loadedClasses[$path];
}
include($path, function($symbols) use ($path) {
foreach ($symbols['classes'] as $className) {
self::$loadedClasses[$path] = $className;
self::$loadedFiles[$className] = $path;
}
});
return self::$loadedClasses[$path];
}//end loadFile()

This approach would be a less disruptive than my prior suggestion which would have added four (4) new *_count() functions and new optional parameters for four (4) existing function vs. this proposal which just adds new optional parameters for four (4) other existing functions. Further, and the signature(s) required for parameters would be much easier to settle on.

I checked the code[2] and if I understand it correctly would (only?) require adding a Zend Observer[3] callback to be notified in zend_bind_class_in_slot() although I still cannot figure out where that observer would need to be added.

Going this approach would also make the userland code in PHPCodeSniffer a lot more efficient than the code you currently must run for every included file simply to determine what symbols were loaded.

What do you think if this alternate approach?

-Mike

[1] https://github.com/php/php-src/blob/18d41502da0da1bb3928e60c41f1b821974c2c01/Zend/zend_builtin_functions.c#L1371
[2] https://github.com/php/php-src/blob/php-8.3.10/Zend/zend_compile.c#L1231-L1269
[3] https://www.linkedin.com/advice/3/what-some-best-practices-common-pitfalls-using-zend