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

PHP8 Hook attributes #1372

Merged
merged 7 commits into from
Nov 2, 2021
Merged

PHP8 Hook attributes #1372

merged 7 commits into from
Nov 2, 2021

Conversation

rpkamp
Copy link
Contributor

@rpkamp rpkamp commented Oct 29, 2021

No description provided.

* @var string[]
*/
private static $classes = array(
AfterFeature::class => 'Behat\Behat\Hook\Call\AfterFeature',
Copy link
Member

Choose a reason for hiding this comment

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

why not using the ::class syntax for array values too ?

Copy link
Contributor Author

@rpkamp rpkamp Oct 29, 2021

Choose a reason for hiding this comment

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

Becuase Behat is still riddles with FQCN strings like these and like to be consistent with existing code.

IMO it would be better to have a separate PR that replaces FQCN strings with ::class "constants" everywhere in one go, rather than mix and match different syntaxes accross the project.

Basically the same reason I'm also using array() here rather than [].

Copy link
Member

Choose a reason for hiding this comment

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

but here, you used ::class for keys. So you are not consistent with your own new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is however consistent with the DefinitionAttributeReader.

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with both approaches, we can address this issue later on - going all-in on ::class syntax in a separate PR seems like a good move.

src/Behat/Hook/BeforeStep.php Outdated Show resolved Hide resolved
There is a finite set of known attributes, this isn't somthing that
should be allowed to be modified in any way.
@pamil pamil merged commit e112914 into Behat:master Nov 2, 2021
@rpkamp rpkamp deleted the hook-attributes branch November 3, 2021 09:49
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.

None yet

3 participants