Skip to content

Conversation

kocsismate
Copy link
Member

No description provided.

@nikic
Copy link
Member

nikic commented Feb 17, 2021

Hm, the stub files for AssertionError and __PHP_Incomplete_Class feel a bit awkward...

@kocsismate
Copy link
Member Author

kocsismate commented Feb 17, 2021

Hm, the stub files for AssertionError and __PHP_Incomplete_Class feel a bit awkward...

First, I was afraid that I'll get the unused function errors again if I put them in zend_builtin_function.stub.php and basic_basic_functions.stub.php, respectively. But I've just realized that the static function declaration helps in this case, so I've just committed a change which puts these classes to their proper place.

@nikic
Copy link
Member

nikic commented Feb 17, 2021

Hm, the stub files for AssertionError and __PHP_Incomplete_Class feel a bit awkward...

First, I was afraid that I'll get the unused function errors again if I put them in zend_builtin_function.stub.php and basic_basic_functions.stub.php, respectively. But I've just realized that the static function declaration helps in this case, so I've just committed a change which puts these classes to their proper place.

As you can see from the failing build, that won't work due to unused static functions. I think in this case it would make more sense to declare the classes inside basic_functions.c, as we also perform all function declarations there.

@kocsismate
Copy link
Member Author

kocsismate commented Feb 17, 2021

As you can see from the failing build, that won't work due to unused static functions.

I didn't notice this warning in my local build output :/

I think in this case it would make more sense to declare the classes inside basic_functions.c, as we also perform all function declarations there.

Is it ok if I only pass the class entries to the original registering functions?

@kocsismate
Copy link
Member Author

I gave up on AssertionError. If I put the assertion_error_ce into the header file as static (so that basic_functions can register it) I get an unused variable error somewhere (https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=14838&view=logs&j=98767452-adc9-546f-46a7-4bcf997bf1db&t=b83fbe93-c624-51ee-5cbf-5d399a1ba3e9&l=22), if I omit static then I get multiple definitions warning.

@nikic
Copy link
Member

nikic commented Feb 22, 2021

@kocsismate The header file should contain something like extern PHPAPI zend_class_entry *php_ce_AssertionError and a C file should contain PHPAPI zend_class_entry *php_ce_AssertionError.

/** @var string|null */
public $name;
public $comment;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should convert these to uninit typed props in a followup.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! I'll soon create one for this and mysqli.

Actually, I wanted to ask you two slightly connected questions to this PR.

  • Couldn't we declare the undeclared properties in ext/dom? (and I found another extension using dynamic properties when working on this PR)
  • I want to backport the missing classes to the PHP-8.0 branch from this PR and the one for ext/spl. Should I include the stub file with the SPL exceptions as well, even though that file wouldn't be used at all...?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we declare the undeclared properties in ext/dom? (and I found another extension using dynamic properties when working on this PR)

Sounds reasonable.

I want to backport the missing classes to the PHP-8.0 branch from this PR and the one for ext/spl. Should I include the stub file with the SPL exceptions as well, even though that file wouldn't be used at all...?

Don't see a problem with that. Though TBH I'm no longer sure we need class declarations on PHP-8.0 at all.

@php-pulls php-pulls closed this in 4c6533c Feb 22, 2021
@kocsismate kocsismate deleted the stub-ce9 branch February 22, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants