Skip to content

PHPC-2146: Refactor typemap struct and BSON encoding/decoding of zvals #1369

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

Merged

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Oct 13, 2022

PHPC-2146

Best reviewed commit by commit. This PR introduces some refactorings that I made while prototyping raw BSON support. No functional changes are introduced as part of this PR. This improves the following:

  • php_phongo_bson_to_zval and php_phongo_bson_to_zval_ex now take a const bson_t* instead of char* and int. Previously, the function would create a new BSON reader to create a bson_t, which is unnecessary as we start out with a bson_t to begin with. php_phongo_bson_data_to_zval and php_phongo_bson_data_to_zval_ex were added to accept a char* before passing this on to php_phongo_bson_to_zval_ex.
  • The php_phongo_bson_typemap was updated to no longer contain a php_phongo_bson_typemap_types and zend_class_entry* for each type. Instead, a new php_phongo_bson_typemap_element struct containing the type and class entry was added. While better structuring data that belongs together, this also makes it easier to add new fields to the type map (in my prototype, I needed to add a potential zval* to this struct which would've made php_phongo_bson_typemap have even more fields without this refactoring.
  • Instantiation of persistable objects in php_phongo_bson_visit_document (including handling for enum on PHP 8.1+) was extracted to php_phongo_bson_init_document_object to make the code more concise.
  • Both php_phongo_bson_visit_document and php_phongo_bson_visit_array were refactored to avoid the duplication of the logic adding the resulting data structure to the parent. This will make it easier to add more options to the type map.
  • The last commit fixes a wrong parameter name in the forward declaration of php_phongo_bson_visit_array.

@alcaeus alcaeus requested a review from jmikola October 13, 2022 06:23
@alcaeus alcaeus self-assigned this Oct 13, 2022
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

Two small requests but LGTM otherwise.

A lot of occurrences start out with a bson_t*, then call bson_get_data on it to pass it to php_phongo_bson_to_zval_ex. This however creates a new bson reader and creates a new bson_t* from the given data. This is unnecessary and we should pass the original bson_t* in these instances.
@alcaeus alcaeus force-pushed the feature/phpc-2146-typemap-struct-refactoring branch from 3be4eb5 to faa8a4a Compare October 24, 2022 09:07
@alcaeus alcaeus merged commit b0b1fd1 into mongodb:master Oct 24, 2022
@alcaeus alcaeus deleted the feature/phpc-2146-typemap-struct-refactoring branch October 24, 2022 10:22
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.

2 participants