Skip to content

Fix support for 32bits PHP #817

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

Merged
merged 3 commits into from
Apr 4, 2025
Merged

Conversation

nicolas-grekas
Copy link
Contributor

Fix #816

@DannyvdSluijs
Copy link
Collaborator

@nicolas-grekas I won't expect these changes to work as the flags are part of a bitmask. Wouldn't it suffice to lower the ERROR_ALL to 0x7FFFFFFF (32 bits signed int max in hex)

@nicolas-grekas
Copy link
Contributor Author

nicolas-grekas commented Apr 4, 2025

I promise this works. The binary representation of -1 is all ones.

public const ERROR_DOCUMENT_VALIDATION = 0x00000001;
public const ERROR_SCHEMA_VALIDATION = 0x00000002;
public const ERROR_NONE = 0;
public const ERROR_ALL = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

self::ERROR_DOCUMENT_VALIDATION | self::ERROR_SCHEMA_VALIDATION also should work here no? Or just hardcode 3 because frankly this is very unlikely to change, so if a new type is added with 4 then you just gotta update the ERROR_ALL.. Doesn't sound that hard 🤷🏻

Suggested change
public const ERROR_ALL = -1;
public const ERROR_ALL = self::ERROR_DOCUMENT_VALIDATION | self::ERROR_SCHEMA_VALIDATION;

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 makes more sense to me to use -1 for an E_ALL level. Like error_reporting(-1) is a common way to turn on all error levels for the engine...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, works for me with -1 too.. Not sure why PHPStan complains though, but I tried on my machine and wasn't able to quiet it down. Seems like the 0xFFFFFFFF makes it give up on some checks.

Copy link
Contributor

@staabm staabm Apr 5, 2025

Choose a reason for hiding this comment

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

Phpstan complains because it doesn’t support int-ranges from binary & and | operator yet.

see phpstan/phpstan#7912

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok thanks, i thought i saw this work but must remember it wrong then :)

@Seldaek
Copy link
Contributor

Seldaek commented Apr 4, 2025

@DannyvdSluijs I know you might be busy with other stuff and that's fine if that's the case, but if you are able to get this into a release in the next couple hours then I could also do a Composer one :)

@DannyvdSluijs
Copy link
Collaborator

I'll go ahead and trust @nicolas-grekas and @Seldaek on this one (both being high valued members in the PHP community).
Pipeline is green, will merge and do a patch release in the next 15 minutes.

@DannyvdSluijs DannyvdSluijs merged commit 074a821 into jsonrainbow:master Apr 4, 2025
15 checks passed
@DannyvdSluijs
Copy link
Collaborator

Version 6.4.1 has been released

@nicolas-grekas
Copy link
Contributor Author

Thank you!

@nicolas-grekas nicolas-grekas deleted the patch-1 branch April 4, 2025 13:10
@Seldaek
Copy link
Contributor

Seldaek commented Apr 4, 2025

Thanks! And just to prove this so https://3v4l.org/VbvJ3 ;)

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.

Version 6 is broken when using a 32bits build of PHP
4 participants