Skip to content

Generate the new class synopsis markup #11809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

kocsismate
Copy link
Member

Following php/phd#77

@kocsismate kocsismate requested a review from Girgias July 27, 2023 18:42
@kocsismate kocsismate changed the title Use new class synopsis generating markup Generate new class synopsis markup Jul 27, 2023
@kocsismate kocsismate changed the title Generate new class synopsis markup Generate the new class synopsis markup Jul 27, 2023
Comment on lines +3054 to +3065
if ($this->type === "class") {
foreach ($this->implements as $interfaceName) {
$interface = $classMap[$interfaceName->toString()] ?? null;
if ($interface === null) {
throw new Exception("Missing implemented interface " . $interfaceName->toString());
}

if ($interface->isException($classMap)) {
return true;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this just to check that Error and Exception have an implements Throwable? that seems kinda overkill? logically every other exception must be extending one of these two classes so maybe it makes more sense to just special case those two classes and let the other loop do its job.

Either case this is also fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to rely on the fact that exceptions currently either extend Error or Exception so that's why I used the more general approach. I'm fine with special casing the logic if you want to.

Copy link
Member

Choose a reason for hiding this comment

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

Eh it's fine with me :)

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM

@kocsismate kocsismate merged commit 566cf37 into php:master Jul 28, 2023
@kocsismate kocsismate deleted the interface-classsynopsis branch July 28, 2023 13:42
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Aug 16, 2023
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.

2 participants