laminas-code icon indicating copy to clipboard operation
laminas-code copied to clipboard

Value generator enum support

Open Rastusik opened this issue 3 years ago • 8 comments

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.

Rastusik avatar Aug 04 '22 15:08 Rastusik

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

Rastusik avatar Aug 04 '22 16:08 Rastusik

Beware that static analysis runs on PHP 7.4.

Ocramius avatar Aug 04 '22 16:08 Ocramius

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

Ocramius avatar Aug 04 '22 16:08 Ocramius

@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.

Rastusik avatar Aug 04 '22 18:08 Rastusik

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

Ocramius avatar Aug 04 '22 19:08 Ocramius

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

Rastusik avatar Aug 05 '22 10:08 Rastusik

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.

Ocramius avatar Aug 05 '22 10:08 Ocramius

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

Rastusik avatar Aug 05 '22 10:08 Rastusik

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

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

Rastusik avatar Oct 04 '22 16:10 Rastusik

No ETA - haven't worked on this yet.

Ocramius avatar Oct 04 '22 17:10 Ocramius

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 avatar Dec 08 '22 01:12 Ocramius

Thanks @Rastusik!

Ocramius avatar Dec 08 '22 01:12 Ocramius

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

Rastusik avatar Dec 08 '22 09:12 Rastusik