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

Value generator enum support #143

Closed

Conversation

Rastusik
Copy link

@Rastusik Rastusik commented Aug 4, 2022

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA no

Description

This patch enables support for enumerations in ValueGenerator, which is currently missing. It is needed to properly generate proxy classes which have enums as default values in method arguments.

@Ocramius Ocramius changed the base branch from develop to 4.7.x August 4, 2022 15:51
@Rastusik Rastusik force-pushed the value_generator_enum_support branch 6 times, most recently from a1a9a61 to bd480f6 Compare August 4, 2022 16:09
Signed-off-by: Rastusik <mfris@pixelfederation.com>
@Rastusik Rastusik force-pushed the value_generator_enum_support branch from bd480f6 to 0252edf Compare August 4, 2022 16:12
@Rastusik
Copy link
Author

Rastusik commented Aug 4, 2022

ok, I have no idea why the static analysis check doesn't work, but it seems like an error in psalm

@Ocramius
Copy link
Member

Ocramius commented Aug 4, 2022

Beware that static analysis runs on PHP 7.4.

@Ocramius
Copy link
Member

Ocramius commented Aug 4, 2022

BTW, dropping PHP 7.4 support is also pretty much OK.

@Rastusik
Copy link
Author

Rastusik commented Aug 4, 2022

@Ocramius what are you suggesting for me to do? I'm not sure what exactly you mean. should I do the 7.4 support drop? I know that the check runs on 7.4, but the code works even on 7.4 so I'm not sure what to do right now.

@Ocramius
Copy link
Member

Ocramius commented Aug 4, 2022

Yeah, try removing 7.4 from composer.json (and update the config.platform)

Rastusik added 2 commits August 4, 2022 22:34
Signed-off-by: Rastusik <mfris@pixelfederation.com>
Signed-off-by: Rastusik <mfris@pixelfederation.com>
@Rastusik Rastusik force-pushed the value_generator_enum_support branch from 355b1ac to f069ead Compare August 4, 2022 20:34
Signed-off-by: Rastusik <mfris@pixelfederation.com>
@Rastusik
Copy link
Author

Rastusik commented Aug 5, 2022

ok so I did what I could but psalm doesn't seem to work correctly


namespace LaminasTest\Code\Generator\TestAsset;

enum TestEnum
Copy link
Member

Choose a reason for hiding this comment

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

Ah, enums were introduced in 8.1, hence the parser failure 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Ok, yeah, I'm gonna pull PHP 8.0 support, but gonna work on the other issues in the milestone first.

@Ocramius
Copy link
Member

Ocramius commented Aug 5, 2022

I think the patch is sound, but in order to raise us to enum support, we'd have to drop PHP 8.1 too.

Psalm correctly infers that the codebase should be verified with PHP 8.0 (compatible):

Run vendor/bin/psalm  --output-format=github --shepherd --stats
Target PHP version: 8.0 (inferred from composer.json)

Unsure what to do here, will need to think about it.

@Rastusik
Copy link
Author

Rastusik commented Aug 5, 2022

yes, that is why I didn't want to do anything more proactively. I'll be waiting for feedback.

@Ocramius Ocramius modified the milestones: 4.7.0, 4.8.0 Sep 13, 2022
@Ocramius Ocramius changed the base branch from 4.7.x to 4.8.x September 13, 2022 10:35
@Rastusik
Copy link
Author

Rastusik commented Oct 4, 2022

Unsure what to do here, will need to think about it.

hey @Ocramius , how about this patch? Is there any ETA?

@Ocramius
Copy link
Member

Ocramius commented Oct 4, 2022

No ETA - haven't worked on this yet.

@Ocramius
Copy link
Member

Ocramius commented Dec 8, 2022

Couldn't push to your fork, so I created #166 instead.

Note how I fixed a bad bug (TM) where the generated value wasn't considering the current namespace :D

@Ocramius Ocramius added Duplicate This issue or pull request already exists and removed Awaiting Maintainer Response labels Dec 8, 2022
@Ocramius Ocramius closed this in #166 Dec 8, 2022
@Ocramius
Copy link
Member

Ocramius commented Dec 8, 2022

Thanks @Rastusik!

@Rastusik
Copy link
Author

Rastusik commented Dec 8, 2022

Note how I fixed a bad bug (TM) where the generated value wasn't considering the current namespace :D

nice :D

thanks for merging @Ocramius

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate This issue or pull request already exists Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants