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

Added fieldtype Bigint #3288

Closed
wants to merge 5 commits into from
Closed

Added fieldtype Bigint #3288

wants to merge 5 commits into from

Conversation

stephanvd
Copy link

We needed fieldtype Bigint, so we added it in. Bigint inherits from Int and is very similar.

@halkyon
Copy link
Contributor

halkyon commented Jul 14, 2014

Few things:

  • Please remove the whitespace additions, might be your editor automatically doing this?
  • Squash the commits together
  • Don't use the ?> end PHP tag in BigInt.php, it's not good practise to do that

If we added this, we'd also need to update the other database adapters to support bigint too:

@@ -0,0 +1,25 @@
<?php
/**
* Represents a signed 32 bit integer field.
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically it's not a 32-bit field, it's an 8-byte (64-bit) field, but I assume this was copied from Int.php. You might also want to mention in the class docs here that PHP running as 32-bit might not work with Bigint properly, as it would convert the value to a float when queried from the database since the value is a 64-bit one.

@simonwelsh
Copy link
Contributor

This will also need pull requests for postgres, sqlite and mssql. Also, should probably be against master versions so that separate pull requests aren't required for those as well.

@stephanvd
Copy link
Author

Thanks for the feedback!

I've now done the following:

  • Commits squashed together.
  • Removed php closing tag.
  • Pull requests for postgres, sqlite and mssql are on their way.
  • Comment on Bigint changed to reflect reality. Warning added.

A few questions:

  • @halkyon Whitespace wasn't added, but unnecessary whitespace was removed. Want me to undo this?
  • @simonwelsh Do you mean I should add another request against master? Or replace this one?

ClayLennart added 4 commits August 20, 2014 13:58
…mework into clay

Tagged 3.1.6

update to 3.1.6

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
@dhensby
Copy link
Contributor

dhensby commented Feb 24, 2015

cherry-picked against 3 branch

@dhensby dhensby closed this Feb 24, 2015
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