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

Allow overriding the PHP 7.2 support for PREG_UNMATCHED_AS_NULL #3226

Merged
merged 2 commits into from
Jul 12, 2024

Conversation

Seldaek
Copy link
Contributor

@Seldaek Seldaek commented Jul 11, 2024

cc @staabm

I am not sure if this is too ugly or too specific for you to care, but in composer/pcre I did build a compat layer for PREG_UNMATCHED_AS_NULL on PHP 7.2/7.3 - so it would be nice if I could enable this flag to get proper return types even on PHP 7.2/7.3

If it's too ugly I understand, and I'll live with it until Composer can drop 7.2 :)

@staabm
Copy link
Contributor

staabm commented Jul 11, 2024

I agree having impl specific feature toggles in phpstan-src looks fishy - especially for such an ancient php version. lets see what ondrey thinks about it

@Seldaek
Copy link
Contributor Author

Seldaek commented Jul 11, 2024

Yeah, I guess alternatively I could simply copy the code in composer/pcre 2.x, it's not that much and shouldn't change too often once stabilized

Edit: that might allow me to remove the conflict on older phpstan too I guess as a bonus..

@staabm
Copy link
Contributor

staabm commented Jul 11, 2024

Edit: that might allow me to remove the conflict on older phpstan too I guess as a bonus..

the RegexArrayShapeMatcher alone does not give you support in older phpstan-src versions. there are a few more parts - like the new *ParameterOutTypeExtension which is required.


lets wait for ondrej. he usually has a clear opinion on such things ;)

@staabm
Copy link
Contributor

staabm commented Jul 12, 2024

just alternative suggestion: instead of adding a new bool flag to the api, we might consider adding a new feature flag config switch in the .neon file doesn't make sense as different apis might be in use at the same time.

please don't adjust the PR until ondrej gave some insight though.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

I'm mostly in favour of this, but what about a custom flag that would be a public constant in RegexArrayShapeMatcher and that would be passed into int $flags? We just need to make sure we're not conflicting with existing PCRE flags.

@Seldaek
Copy link
Contributor Author

Seldaek commented Jul 12, 2024

Yeah that's a great idea to avoid polluting the arg list, I'll investigate.

@Seldaek
Copy link
Contributor Author

Seldaek commented Jul 12, 2024

Ok I think that's what you meant?

@staabm
Copy link
Contributor

staabm commented Jul 12, 2024

the phpdoc type needs a fix:

* @param int-mask<PREG_OFFSET_CAPTURE|PREG_UNMATCHED_AS_NULL>|null $flags

/**
* Pass this into $flagsType as well if the library supports emulating PREG_UNMATCHED_AS_NULL on PHP 7.2 and 7.3
*/
public const PREG_UNMATCHED_AS_NULL_ON_72_73 = 2048;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The highest in use currently is 256, so I chose 2048 to leave 512/1024 as gaps if PHP should add new constants there.

Comment on lines +65 to +66
/** @var int-mask<PREG_OFFSET_CAPTURE | PREG_UNMATCHED_AS_NULL | self::PREG_UNMATCHED_AS_NULL_ON_72_73> $flags */
$flags = $flagsType->getValue() & (PREG_OFFSET_CAPTURE | PREG_UNMATCHED_AS_NULL | self::PREG_UNMATCHED_AS_NULL_ON_72_73);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow phpstan thinks it's an int<0, 2816> as that's the value on the right. Could be nice if it was smarter here but I guess bitwise use is rare enough that it's not worth that much time spent optimizing for them.

The var annotation fixed it though.

@ondrejmirtes ondrejmirtes merged commit 3a32992 into phpstan:1.11.x Jul 12, 2024
450 of 454 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@Seldaek Seldaek deleted the patch-10 branch July 13, 2024 11:14
@Seldaek
Copy link
Contributor Author

Seldaek commented Jul 13, 2024

Cool, implemented in composer/pcre@088f752 and it works great!

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