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

Corrected PHP type for "decimal" mapping type #744

Merged
merged 3 commits into from
Aug 10, 2013
Merged

Corrected PHP type for "decimal" mapping type #744

merged 3 commits into from
Aug 10, 2013

Conversation

jbruni
Copy link
Contributor

@jbruni jbruni commented Aug 3, 2013

"Basic Mapping" documentation says:
"decimal: Type that maps a SQL DECIMAL to a PHP string."

"Basic Mapping" documentation says:
"decimal: Type that maps a SQL DECIMAL to a PHP string."
@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2587

We use Jira to track the state of pull requests and the versions they got
included in.

@jbruni
Copy link
Contributor Author

jbruni commented Aug 3, 2013

@beberlei
Copy link
Member

beberlei commented Aug 3, 2013

@jbruni Please fix the tests you broke, see The Travis result

@jbruni
Copy link
Contributor Author

jbruni commented Aug 3, 2013

@beberlei Done. (As you certainly know, the build still fails due to issues unrelated to this PR.) Thank you.

@jbruni
Copy link
Contributor Author

jbruni commented Aug 3, 2013

Look - I am assuming the documentation is correct, and "decimal" mapping type should really map to "string" PHP type. This not only sounds as the proper behaviour to me, and it is also the desirable/wanted behaviour to me.

I don't know about Doctrine internals related to this. The message for the documentation update commit (linked above) says: "Updated the decimal type mapping have a string on the php side (current behavior)"

In a Symfony 2 project, I ran php app/console doctrine:generate:entity and when seeing the generated PHP code, I was surprised to see "var float" in the docblock, when expecting to see "var string" - as stated in the documentation I was following.

This is why I opened this PR. Either the generator need this fix, or the documentation! I can't tell, but I really expect my "decimal" to behave as "string", not "float"!

Thank you.

@stof
Copy link
Member

stof commented Aug 3, 2013

The issue is in the generator. the actual value is indeed a string

@jbruni
Copy link
Contributor Author

jbruni commented Aug 3, 2013

Great. Indeed.

I've investigated the internals... now I am aware of how it works. It is simple! Well done!

Anyway, I've found this commit:

doctrine/dbal@9f51afa#lib/Doctrine/DBAL/Types/DecimalType.php

Which made DecimalType.php like this:

https://github.com/doctrine/dbal/blob/9f51afa210d5c0165dfe6c622e07e42aa54c09fb/lib/Doctrine/DBAL/Types/DecimalType.php

Notice that the comment was not updated... so, I am opening a new tiny PR for Doctrine DBAL:

doctrine/dbal#353

Thanks!

@jbruni
Copy link
Contributor Author

jbruni commented Aug 6, 2013

I've added a useless commit only to perform a git push and force a new build on Travis.

(This PR build was still failing due to the "DDC-2524" issue.)

beberlei added a commit that referenced this pull request Aug 10, 2013
Corrected PHP type for "decimal" mapping type
@beberlei beberlei merged commit 610e189 into doctrine:master Aug 10, 2013
@jbruni jbruni deleted the patch-1 branch August 17, 2013 11:45
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.

4 participants