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

Fix zend framework 2 db multi queries should default to min/max 1/500 #1148

Merged
merged 1 commit into from Oct 30, 2014

Conversation

Projects
None yet
3 participants
@gerardroche
Contributor

gerardroche commented Oct 29, 2014

@msmith-techempower

This comment has been minimized.

Show comment
Hide comment
@msmith-techempower

msmith-techempower Oct 29, 2014

Member

This looks fine, but there are still some warnings in the travis output: https://travis-ci.org/TechEmpower/FrameworkBenchmarks/jobs/39345008

I can merge now, or you can commit more fixes.

Member

msmith-techempower commented Oct 29, 2014

This looks fine, but there are still some warnings in the travis output: https://travis-ci.org/TechEmpower/FrameworkBenchmarks/jobs/39345008

I can merge now, or you can commit more fixes.

@gerardroche

This comment has been minimized.

Show comment
Hide comment
@gerardroche

gerardroche Oct 30, 2014

Contributor

What exactly are the warnings for? My guess is that it's because the json should have integer values rather than string integers. If this the case I say merge it as is because this is how 90+% of zf users would encode json using the framework.

The Zend Framework version 1of these benchmarks displays the same watnings too, so same as above. #874

Contributor

gerardroche commented Oct 30, 2014

What exactly are the warnings for? My guess is that it's because the json should have integer values rather than string integers. If this the case I say merge it as is because this is how 90+% of zf users would encode json using the framework.

The Zend Framework version 1of these benchmarks displays the same watnings too, so same as above. #874

@hamiltont

This comment has been minimized.

Show comment
Hide comment
@hamiltont

hamiltont Oct 30, 2014

Contributor

My guess is that it's because the json should have integer values rather than string integers

Correct, that's the only thing it's warning about. This LGTM

Contributor

hamiltont commented Oct 30, 2014

My guess is that it's because the json should have integer values rather than string integers

Correct, that's the only thing it's warning about. This LGTM

@hamiltont

This comment has been minimized.

Show comment
Hide comment
@hamiltont

hamiltont Oct 30, 2014

Contributor

For what it's worth, the warning is not being issued because TFB developers have some preferred representation of numbers in JSON - it's warning you because this might cause your framework to bump up against the physical limits of the networking hardware, as the quotation marks consume a few extra bytes per request.

If this is the natural JSON representation for zend I would totally ignore these warnings, unless Zend gets the top 2 or 3 spot in the preview data for the next round (then definitely optimize it and go for one!)

Contributor

hamiltont commented Oct 30, 2014

For what it's worth, the warning is not being issued because TFB developers have some preferred representation of numbers in JSON - it's warning you because this might cause your framework to bump up against the physical limits of the networking hardware, as the quotation marks consume a few extra bytes per request.

If this is the natural JSON representation for zend I would totally ignore these warnings, unless Zend gets the top 2 or 3 spot in the preview data for the next round (then definitely optimize it and go for one!)

hamiltont added a commit that referenced this pull request Oct 30, 2014

Merge pull request #1148 from gerardroche/zf2-fix-db-multi
PHP: Fix zend framework 2 min/max 1/500

@hamiltont hamiltont merged commit 7064c54 into TechEmpower:master Oct 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@hamiltont

This comment has been minimized.

Show comment
Hide comment
@hamiltont

hamiltont Oct 30, 2014

Contributor

LGTM, thanks again @gerardroche !

Contributor

hamiltont commented Oct 30, 2014

LGTM, thanks again @gerardroche !

@gerardroche gerardroche referenced this pull request Apr 16, 2015

Closed

Errors in PHP tests #520

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