Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Excess arguments for internal functions »

Excess arguments for internal functions

php.internals

Christoph Becker

8 years ago
Hi! Is there any particular reason why internal functions raise a warning/an error regarding excess arguments, unless they are not supposed to receive any arguments at all? In other words, why don't we use ZEND_PARSE_PARAMETERS_NONE() (except for net_get_interfaces())?
-- Christoph M. Becker

Nikita Popov

8 years ago
On Mon, Jul 2, 2018 at 11:00 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote:
> Hi! > > Is there any particular reason why internal functions raise a warning/an > error regarding excess arguments, unless they are not supposed to > receive any arguments at all? In other words, why don't we use > ZEND_PARSE_PARAMETERS_NONE() (except for net_get_interfaces())? >
The more common macro for this is zend_parse_parameters_none(), which a quick grep shows to be used by 519 functions. If a function accepts no arguments and does not use zend_parse_parameters_none(), that's generally a bug. Nikita

Christoph Becker

8 years ago
On 02.07.2018 at 23:16, Nikita Popov wrote:
> On Mon, Jul 2, 2018 at 11:00 PM, Christoph M. Becker <cmbecker69@gmx.de> > wrote: > >> Is there any particular reason why internal functions raise a warning/an >> error regarding excess arguments, unless they are not supposed to >> receive any arguments at all? In other words, why don't we use >> ZEND_PARSE_PARAMETERS_NONE() (except for net_get_interfaces())? > > The more common macro for this is zend_parse_parameters_none(), which a > quick grep shows to be used by 519 functions. If a function accepts no > arguments and does not use zend_parse_parameters_none(), that's generally a > bug.
Ah, thanks! I couldn't find zend_parse_parameters_none() in README.PARAMETER_PARSING_API[1] – guess it should be added there. Anyhow, I've noticed this due to bug 33502[2], and indeed for output_reset_rewrite_vars()[3] there is still no ZPP in place. Then I've checked func_num_args()[4] (which just came to mind), and found that it doesn't do any ZPP either, as well as other parameterless functions defined immediately before it. Should these (and other occurences, if there are any) be fixed for PHP-7.1, or for master only? I'm somewhat concerned regarding the potential BC break, particularly with strict_types. [1] <https://github.com/php/php-src/blob/a7028d96719fef1939eb3bf14ddb05491ef4e09f/README.PARAMETER_PARSING_API> [2] <https://bugs.php.net/bug.php?id=33502> [3] <https://github.com/php/php-src/blob/f2c4f06f84fd3a9fbb241dd84179eaa591f196a4/main/output.c#L1542-L1552> [4] <https://github.com/php/php-src/blob/2543e61aed67add7522e0b4cdf9a13cf3e441f6f/Zend/zend_builtin_functions.c>
-- Christoph M. Becker

Nikita Popov

8 years ago
On Mon, Jul 2, 2018 at 11:45 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote:
> On 02.07.2018 at 23:16, Nikita Popov wrote: > > > On Mon, Jul 2, 2018 at 11:00 PM, Christoph M. Becker <cmbecker69@gmx.de> > > wrote: > > > >> Is there any particular reason why internal functions raise a warning/an > >> error regarding excess arguments, unless they are not supposed to > >> receive any arguments at all? In other words, why don't we use > >> ZEND_PARSE_PARAMETERS_NONE() (except for net_get_interfaces())? > > > > The more common macro for this is zend_parse_parameters_none(), which a > > quick grep shows to be used by 519 functions. If a function accepts no > > arguments and does not use zend_parse_parameters_none(), that's > generally a > > bug. > > Ah, thanks! I couldn't find zend_parse_parameters_none() in > README.PARAMETER_PARSING_API[1] – guess it should be added there. > > Anyhow, I've noticed this due to bug 33502[2], and indeed for > output_reset_rewrite_vars()[3] there is still no ZPP in place. Then > I've checked func_num_args()[4] (which just came to mind), and found > that it doesn't do any ZPP either, as well as other parameterless > functions defined immediately before it. > > Should these (and other occurences, if there are any) be fixed for > PHP-7.1, or for master only? I'm somewhat concerned regarding the > potential BC break, particularly with strict_types. >
I'd go for master only as it's technically BC breaking. Some people have plans to get rid of the warning/error on too many arguments to internal functions to make them consistent with user functions, so maybe this will become unnecessary in the future. But given the current state of things, it would certainly be good to add zend_parse_parameters_none() calls where they are missing. Nikita

Sara Golemon

8 years ago
On Tue, Jul 3, 2018 at 5:11 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:
> I'd go for master only as it's technically BC breaking. >
Ditto this.
> Some people have plans to get rid of the warning/error on too many > arguments to internal functions to make them consistent with user > functions, so maybe this will become unnecessary in the future. But given > the current state of things, it would certainly be good to add > zend_parse_parameters_none() calls where they are missing. >
IMO, we should go the other way. Deprecate excess args in userspace functions (making them an error in 8.0). We've had formal variadics in userspace for a long time (5.5 IIRC), this is a solvable problem. I also know that's not going to happen (for well defensible reasons) and I accept that. Felt the need to state my piece though. -Sara

Stas Malyshev

8 years ago
Hi!
> IMO, we should go the other way. Deprecate excess args in userspace > functions (making them an error in 8.0). We've had formal variadics > in userspace for a long time (5.5 IIRC), this is a solvable problem.
I think it's premature. We do have variadics, since 5.6 (https://github.com/php/php-src/blob/PHP-5.6/UPGRADING) but that's not "a long time" - not for PHP code and for the speeds people upgrade. There are still major code bases running on 5.x. There's still tons of code out there which does variadics the old-fashioned way, and I don't see any reason to break it - we do not gain anything from it. We should not force people to use variadics just for variadics' sake where their old code is already working. My position remains - there should not be BC breaks that don't gain people some benefit in some way. For example, there should not be BC breaks just to promote a new feature, even if it's way better than the old way - as long as the old way still works fine and does not cause trouble. This seems to be such case - non-variadic calls still work, and as far as I see, there's no improvement that allowing extra arguments breaks or makes harder. I think I'd rather allow extra args everywhere than ban them in user functions. But just having a warning in zero-arg internals is fine too - there we *can* know these weren't intended to be variadics, so we *can* know it's probably a mistake, unlike with user functions where in the generic case we don't know that.
-- Stas Malyshev smalyshev@gmail.com