Skip to content
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

Add Secured as attribute for PHP >= 8.0 #37

Merged
merged 2 commits into from
Nov 17, 2022
Merged

Conversation

haltuf
Copy link
Contributor

@haltuf haltuf commented Nov 15, 2022

PHP 8.0 brings a new backward compatible way to replace annotations.

This PR adds support for attribute #[Secured] or #[\Nextras\Application\UI\Secured] to be used instead of annotation @secured.

@@ -41,7 +41,7 @@ public function signalReceived(string $signal): void

if (method_exists($this, $method)) {
$reflection = new \ReflectionMethod($this, $method);
$secured = Nette\Application\UI\ComponentReflection::parseAnnotation($reflection, 'secured') !== NULL;
$secured = Nette\Application\UI\ComponentReflection::parseAnnotation($reflection, 'secured') !== NULL || count($reflection->getAttributes(Secured::class)) > 0;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be iffed / or possibly, just remove support for PHP<8.0.

Copy link
Member

Choose a reason for hiding this comment

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

Weird that phpstan is ok with this.

Copy link
Contributor Author

@haltuf haltuf Nov 15, 2022

Choose a reason for hiding this comment

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

Dropping PHP<8.0 should be your call in this project :-)
I will add the iffs. What confused me is that tests passed for PHP<8, but that's because tests currently don't cover this part of code, maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hrach Added method_exists.
Also added test coverage for signalReceived, wasn't covered before.
Only covers @secured, not #[Secured] at the moment.

@hrach hrach merged commit 63c7481 into nextras:master Nov 17, 2022
@hrach
Copy link
Member

hrach commented Nov 17, 2022

Awesome, thank you.

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