doctrine
Fix bigint PHP_INT_MIN/PHP_INT_MAX string to int convert
| Q | A |
|---|---|
| Type | bug |
| Fixed issues | n/a |
Summary
Resolve https://github.com/doctrine/dbal/pull/6177#discussion_r1613456751 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.
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.
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.
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
Sorry, but that implementation is way too complicated. I don't want to maintain this.
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.
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.
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?
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, ...
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.
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 https://github.com/doctrine/dbal/pull/6410/commits/2cc0fae35310d65b013b65f68e6aa05e40ecb048
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 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.
@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?
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?
I don't understand the problem, and I do not see an explanation about it anywhere.
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.
I won't block this anymore but I don't get the fix, feel free to deal with this if you understand it @derrabus
As soon as the unnecessary changes to the test have been reverted, I'm fine with the PR.