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 Support for Fetch Modes #4

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Added Support for Fetch Modes #4

wants to merge 17 commits into from

Conversation

iliaal
Copy link

@iliaal iliaal commented Dec 15, 2019

Couple of feature enhancements to simplify data retrieval

  1. Added ability to fetch dates as strings (date as Y-m-d and datetime as Y-m-d H:i:s similar to how it is returned by HTTP client) via DATE_AS_STRINGS mode
  2. Added ability to fetch single value via FETCH_ONE mode
  3. Added ability to retrieve all volumes from single column as an basic array of values via FETCH_COLUMN mode
  4. Added ability to fetch results as an associated array (key-value-pair) via FETCH_KEY_PAIR mode using col1 as index and col2 as value.

Also added SeasClickException class so that exceptions thrown are specific to extension as opposed to using generic Exception class.

@wujunze wujunze requested review from Neeke and aiwhj December 16, 2019 04:32
Copy link
Member

@769344359 769344359 left a comment

Choose a reason for hiding this comment

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

why remove shutdown callback ?

@@ -167,9 +157,9 @@ zend_module_entry SeasClick_module_entry =
SEASCLICK_RES_NAME,
SeasClick_functions,
PHP_MINIT(SeasClick),
PHP_MSHUTDOWN(SeasClick),
Copy link
Member

Choose a reason for hiding this comment

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

why remove shutdown callback?

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't do anything, so recommended approach is to not invlude PHP_MSHUT / PHP_MINIT unless they do something

@@ -98,6 +99,9 @@ const zend_function_entry SeasClick_methods[] =
PHP_FE_END
};

#define REGISTER_SC_CLASS_CONST_LONG(const_name, value) \
zend_declare_class_constant_long(SeasClick_ce, const_name, sizeof(const_name)-1, (zend_long)value);
Copy link
Member

Choose a reason for hiding this comment

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

Please do some compatibility tests, such as zend_long type does not support PHP 5 version

Copy link
Author

Choose a reason for hiding this comment

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

fixed

SeasClick.cpp Outdated
convertToZval(col2, block[1], row, "", 0, fetch_mode|SC_FETCH_ONE);

if (Z_TYPE_P(col1) == IS_LONG) {
zend_hash_index_update(Z_ARRVAL_P(return_value), Z_LVAL_P(col1), col2);
Copy link
Member

Choose a reason for hiding this comment

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

Compatibility

#define zend_hash_index_update(ht, h, pData, nDataSize, pDest) _zend_hash_index_update_or_next_insert(ht, h, pData, nDataSize, pDest, HASH_UPDATE ZEND_FILE_LINE_CC)

This is the definition of PHP 5 version

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@iliaal iliaal requested a review from 769344359 April 6, 2020 12:02
@iliaal iliaal requested a review from aiwhj April 6, 2020 12:47
@guba-odudkin
Copy link

@769344359 @Neeke Any chance to get it merged?

@Rock-Lee-520
Copy link
Member

@769344359 @Neeke Any chance to get it merged?

it will have been merged when i finsh unit tests!

@Rock-Lee-520
Copy link
Member

image

@guba-odudkin could you please help me to fix the conflicts ? it change a lot of file, i can not merge it .

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.

None yet

5 participants