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.

Sign up for GitHub

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

Jump to bottom

Fix bigint PHP_INT_MIN/PHP_INT_MAX string to int convert #6410

Open
wants to merge 7 commits into
base: 4.0.x
Choose a base branch
from
Open

Fix bigint PHP_INT_MIN/PHP_INT_MAX string to int convert #6410

wants to merge 7 commits into from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented May 28, 2024 •
edited
Loading

Q A
Type bug
Fixed issues n/a

Summary

Resolve #6177 (comment) discussion and related original #6177.

Whole native php int range is guaranteed to be supported per https://www.doctrine-project.org/projects/doctrine-dbal/en/4.0/reference/types.html#bigint docs.

@mvorisek mvorisek force-pushed the fix_min_max_bigint_cast branch 3 times, most recently from 5b74171 to dcbddf3 Compare May 28, 2024 10:16
@mvorisek mvorisek marked this pull request as ready for review May 28, 2024 10:17
@mvorisek
Copy link
Contributor Author

@derrabus can you please review?

@derrabus
Copy link
Member

Please don't ping me on PRs please.

@mvorisek
Copy link
Contributor Author

I pinged you because of you authored the original PR, sorry.

@mvorisek mvorisek force-pushed the fix_min_max_bigint_cast branch 3 times, most recently from e22cc52 to dc5cd73 Compare May 28, 2024 15:16
@mvorisek mvorisek marked this pull request as draft May 28, 2024 15:18
@mvorisek mvorisek force-pushed the fix_min_max_bigint_cast branch 5 times, most recently from 347ded4 to 4c3258b Compare May 28, 2024 15:45
@mvorisek mvorisek marked this pull request as ready for review May 28, 2024 16:06
@derrabus
Copy link
Member

Please keep in mind that this piece of code will be executed on a hot path. If I hydrate thousands of entities with bigint fields, I don't want to execute preg_match() for each of them. Your implementation is too expensive.

@mvorisek
Copy link
Contributor Author

mvorisek commented May 28, 2024 •
edited
Loading

Please keep in mind that this piece of code will be executed on a hot path. If I hydrate thousands of entities with bigint fields, I don't want to execute preg_match() for each of them. Your implementation is too expensive.

PCRE JIT is very fast, but yes, the regex replace is possible to be coded /wo regex and I alredy considered that option because only limited number (4096) are cached. I will rework the code.

@mvorisek mvorisek marked this pull request as draft May 28, 2024 19:57
@mvorisek mvorisek force-pushed the fix_min_max_bigint_cast branch 2 times, most recently from 70d3683 to fc86908 Compare May 28, 2024 20:15
@mvorisek mvorisek marked this pull request as ready for review May 28, 2024 20:18
@mvorisek
Copy link
Contributor Author

Please keep in mind that this piece of code will be executed on a hot path. If I hydrate thousands of entities with bigint fields, I don't want to execute preg_match() for each of them. Your implementation is too expensive.

done

@derrabus
Copy link
Member

Sorry, but that implementation is way too complicated. I don't want to maintain this.

@mvorisek
Copy link
Contributor Author

The implementation is minimal, we strip leading plus sign, zeros and trailing zeros after decimal point. If the number is then castable into int without precision loss, we cast it.

The leading/trailing zeros should be stripped because the input number can come from sources with explicit digits/decimal configured. This behaviour is tested and I do not think it can be implemented simpler. If you have an idea how to implemenet this simpler/better, I am of course ready for your ideas.

@mvorisek
Copy link
Contributor Author

If leading/trailing zeros should be supported, the impl. is minimal IMO. I would be happy if this can either be merged as is or please let me know how to fix the min/max issue differently.

@derrabus
Copy link
Member

Which DBMS formats 2^31-1 with leading zeros?

@mvorisek
Copy link
Contributor Author

I tested all DBs using https://dbfiddle.uk/6OSky-ka and none DB vendor prepend leading zeros even for DECIMAL type by default.

So you want me to remove the "leading zeros accepting" code in order to save a few lines of code?

@derrabus
Copy link
Member

🤷‍♂️

@mvorisek
Copy link
Contributor Author

I am asking as the integer DBAL Type class can be used to load any value, the value might some from user data, formatted string column, ...

@derrabus
Copy link
Member

I am asking as the integer DBAL Type class can be used to load any value

Sure, but it's not meant to be used on any value. And I refuse to build workarounds to accommodate misuse.

@mvorisek
Copy link
Contributor Author

I am asking as the integer DBAL Type class can be used to load any value

Sure, but it's not meant to be used on any value. And I refuse to build workarounds to accommodate misuse.

simplified in 2cc0fae

derrabus
derrabus reviewed Jun 17, 2024
View reviewed changes
src/Types/BigIntType.php Outdated Show resolved Hide resolved
derrabus
derrabus reviewed Jun 17, 2024
View reviewed changes
src/Types/BigIntType.php Outdated Show resolved Hide resolved
mvorisek
mvorisek commented Jun 17, 2024
View reviewed changes
src/Types/BigIntType.php
assert(
is_string($value),
'DBAL assumes values outside of the integer range to be returned as string by the database driver.',
);

if ($value === (string) (int) $value) {
return (int) $value;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

psalm is wrong here - vimeo/psalm#10995

tell me how to fix the CI

@derrabus
Copy link
Member

in my company we use DBAL Types to cast data even from APIs for example

But that's an off-label use and I hope you're fully aware of that. If your goal is to normalize data coming from arbitrary external sources, there are libraries tailored to that task, like Symfony Serializer for example. DBAL does neither want nor need to compete with such libraries. If our type conversion system does not work for your clearly undocumented use-case, please use something else. Honestly.

If you however persist on minimalistic impl., I am ok to give my code away even if I am not happy with it.

Okay, I'd like to settle this once and for all. I really appreciate the dedication with which you contribute to our libraries. I really do. However, we want to keep our libraries focused and maintainable. And your initial proposal was anything but focused and maintainable. You've proposed to merge a piece of code that nobody but you will ever need and which at the same time would significantly slow down our type conversion for everybody else.

The time that we maintainers can spend on open source work is finite and mostly unpaid. The time that I waste on unnecessary discussions – like this one – is everybody's loss because I cannot spend it on reviewing other contributions, yours included.

@greg0ire
Copy link
Member

I don't think this is going anywhere, sorry.

@greg0ire greg0ire closed this Jun 20, 2024
@mvorisek
Copy link
Contributor Author

@greg0ire what you you mean, there is a bug and I coded the tests and the fix, please reopen this PR and let me know what changes, if any, you want.

@mvorisek
Copy link
Contributor Author

@greg0ire with Alexander I got to a point when this PR is lighweight and passing the tests. Can you please clarify "I don't think this is going anywhere" into a feedback I can act on? Can you please reopen this PR?

@greg0ire
Copy link
Member

Clarification: I don't think there is anything you can do.

@mvorisek
Copy link
Contributor Author

Sorry, do you understand the problem, are you aware that valid max. int 2^64-1 (and min.) value is currently not properly casted to int type?

@greg0ire
Copy link
Member

I don't understand the problem, and I do not see an explanation about it anywhere.

@derrabus
Copy link
Member

I don't understand the problem

The problem ist that if the DB would return PHP_INT_MAX as string, we would not convert it to int although one might expect that.

@greg0ire greg0ire reopened this Jul 11, 2024
@greg0ire
Copy link
Member

I won't block this anymore but I don't get the fix, feel free to deal with this if you understand it @derrabus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mvorisek @derrabus @greg0ire