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 php8.1 enum support #54

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add php8.1 enum support #54

wants to merge 10 commits into from

Conversation

tdumalin
Copy link

@tdumalin tdumalin commented Mar 9, 2022

Hi,

I've created an issue on StateMachineBundle (#70) but i realized that it's on this repo that we have to work.
So this is an implementation for php8.1 enum support, let me know what do you think.

@lchrusciel
Copy link
Collaborator

Hey @tdumalin,

thanks a lot for your contribution! I would need to sit into how to properly solve it here. Also, such code would require to be available only with PHP8.1 enabled

@tdumalin
Copy link
Author

tdumalin commented Mar 9, 2022

@lchrusciel

Yes indeed, I've seen the build errors and parseError could not be avoided in php < 8.1. The only solution would be to add php version in composer.json but it will be very tricky to manage.
Let me know if you've found a solution 👍

@coldic3
Copy link
Contributor

coldic3 commented Mar 9, 2022

Hey! What do you guys think about skipping these tests for PHP <8.1? Skipping examples can be used for that:

if (version_compare(PHP_VERSION, '8.1.0', '<')) {
    throw new \PhpSpec\Exception\Example\SkippingException('Requires at least PHP 8.1.');
}

@tdumalin
Copy link
Author

tdumalin commented Mar 9, 2022

@coldic3 ,
I just pushed something quite similar with version compare,
wait and see if the build will pass

Copy link
Contributor

@coldic3 coldic3 left a comment

Choose a reason for hiding this comment

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

Hey @tdumalin, I see tests passed 🙂

I've added some small comments. I also wonder if the states key in the configuration might not be required while using enum 🤔

spec/SM/DummyEnumState.php Show resolved Hide resolved
spec/SM/StateMachine/StateMachineSpec.php Outdated Show resolved Hide resolved
spec/SM/StateMachine/StateMachineSpec.php Outdated Show resolved Hide resolved
src/SM/StateMachine/StateMachine.php Outdated Show resolved Hide resolved
src/SM/StateMachine/StateMachine.php Outdated Show resolved Hide resolved
src/SM/StateMachine/StateMachine.php Outdated Show resolved Hide resolved
src/SM/StateMachine/StateMachine.php Outdated Show resolved Hide resolved
src/SM/StateMachine/StateMachine.php Outdated Show resolved Hide resolved
spec/SM/StateMachine/StateMachineSpec.php Show resolved Hide resolved
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.

3 participants