Skip to content

Add ZEND_PARSE_PARAMETERS_NONE to private constructors#21344

Closed
jordikroon wants to merge 2 commits intophp:masterfrom
jordikroon:zzp-private-constructor
Closed

Add ZEND_PARSE_PARAMETERS_NONE to private constructors#21344
jordikroon wants to merge 2 commits intophp:masterfrom
jordikroon:zzp-private-constructor

Conversation

@jordikroon
Copy link
Contributor

@jordikroon jordikroon commented Mar 4, 2026

Formally this should also be added to private constructors, which I stumbled on when blocking __PHP_Incomplete_Class.

This PR adds ZEND_PARSE_PARAMETERS_NONE() to all private constructors. No more follow-up PR's to be expected.

@jordikroon jordikroon changed the title add ZEND_PARSE_PARAMETERS_NONE to private constructors Add ZEND_PARSE_PARAMETERS_NONE to private constructors Mar 4, 2026
@jordikroon
Copy link
Contributor Author

Welp I didn't expect any tests to fail, but the ReflectionAttribute construction test should not have parameters. So those have been removed from the test.

@DanielEScherzer
Copy link
Member

I'm probably in the minority, but I'm going to suggest that this doesn't help - if someone tries calling a private constructor, it is more important to tell them that the constructor is private than to first validate the arguments

@iluuu1994
Copy link
Member

I agree with @DanielEScherzer. No point in telling the user how to fix a broken function call that they'll have to remove anyway.

@jordikroon
Copy link
Contributor Author

I understand both point of views. The reason for this PR is because @ndossche mentioned it technically needs to be added to be fully correct. To make it match the stub I presume. To that part I agree. But I also understand that it might give a false positive when using a wrong implementation.

That said it's likely also best to not add this line to the RFC PR. For me it's most important to have consistent (new) code. With many people reviewing code it's sometimes difficult to do what is best given contrary opinions. Which I fully understand.

Given the difference in opinion it's probably best to not merge this.

@jordikroon jordikroon closed this Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants