Create an object oriented interface for Hash Context #611

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

realityking commented Mar 3, 2014

I liked the idea, recently mentioned on internals, to remove the use of resources on use object oriented APIs instead.

This pull request does just that, by introducing the class HashContext. It's targeted at master, since this should go into PHP 5.7.

Tests all pass, one required modification as it relied on the output of var_dump. The B/C break is very small, code that relies on checking is_resource() fails and needs to be changed to also do `ìnstanceof HashContext``.

A quick search on GitHub revealed no project that uses HashContext in the global namespace.

@nikic nikic and 1 other commented on an outdated diff Mar 3, 2014

ext/hash/hash.c
+ return 1;
+}
+
+PHP_METHOD(HashContext, __construct)
+{
+ char *algo, *key = NULL;
+ int algo_len, key_len = 0;
+ long options = 0;
+ hashContext_object *intern;
+ zend_error_handling error_handling;
+
+ if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|ls", &algo, &algo_len, &options, &key, &key_len) == FAILURE) {
+ return;
+ }
+
+ zend_replace_error_handling(EH_THROW, NULL, &error_handling TSRMLS_CC);
@nikic

nikic Mar 3, 2014

Owner

Error handling must be replaced before the zpp call already, otherwise you'll end up with new returning null.

@realityking

realityking Mar 3, 2014

Contributor

Fixed that, thanks.

@nikic nikic and 1 other commented on an outdated diff Mar 3, 2014

ext/hash/hash.c
+Initialize a hashing context */
+PHP_FUNCTION(hash_init)
+{
+ char *algo, *key = NULL;
+ int algo_len, key_len = 0;
+ long options = 0;
+ hashContext_object *hash;
+
+ if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|ls", &algo, &algo_len, &options, &key, &key_len) == FAILURE) {
+ return;
+ }
+
+ Z_TYPE_P(return_value) = IS_OBJECT;
+ object_init_ex(return_value, hashContext_ce);
+ Z_SET_REFCOUNT_P(return_value, 1);
+ Z_UNSET_ISREF_P(return_value);
@nikic

nikic Mar 3, 2014

Owner

Why are you explicitly setting refcount and isref here? This code should be reducible to

Z_TYPE_P(return_value) = IS_OBJECT;
Z_OBJVAL_P(return_value) = hashContext_create_object_handler(hashContext_ce TSRMLS_CC);

Or, if you want to go implicitly through object_init, then just object_init_ex(return_value, hashContext_ce); should do. It will automatically set IS_OBJECT.

@realityking

realityking Mar 3, 2014

Contributor

I oriented myself on this code: http://lxr.php.net/xref/PHP_5_5/ext/date/php_date.c#php_date_instantiate, but object_init_ex seem like a good fit. Will look into it.

@realityking

realityking Mar 3, 2014

Contributor

Just realized that I'm already using object_init_ex, I'll remove the manual setting of IS_OBJECT.

@nikic

nikic Mar 3, 2014

Owner

The SET_REFCOUNT and UNSET_ISREF are unnecessary as well.

@realityking

realityking Mar 3, 2014

Contributor

I'll take your word for it. How about the use in hash_copy()?

@nikic

nikic Mar 3, 2014

Owner

Same applies there. return_value is guaranteed to have refcount=1 and isref=0 initially ;)

@realityking

realityking Mar 3, 2014

Contributor

Fixed, thanks.

@nikic nikic commented on the diff Mar 3, 2014

ext/hash/hash.c
return;
}
- ZEND_FETCH_RESOURCE(hash, php_hash_data*, &zhash, -1, PHP_HASH_RESNAME, php_hash_le_hash);
+ hash = zend_object_store_get_object(zhash TSRMLS_CC);
hash->ops->hash_update(hash->context, (unsigned char *) data, data_len);
@nikic

nikic Mar 3, 2014

Owner

This and all the other code assumes that the context was properly initialized. However, there is no guarantee that the __construct method was called, so hash->ops, hash->context etc could all be NULL here. I suggest throwing an exception in such a case.

@realityking

realityking Mar 3, 2014

Contributor

Good point. But I was under the impression, that exceptions are not permitted in procedural code? So this should probably be an E_RECOVERABLE_ERROR.

@nikic nikic commented on the diff Mar 3, 2014

ext/hash/hash.c
}
digest[digest_len] = 0;
efree(hash->context);
hash->context = NULL;
- /* zend_list_REAL_delete() */
- if (zend_hash_index_find(&EG(regular_list), Z_RESVAL_P(zhash), (void *) &le)==SUCCESS) {
- /* This is a hack to avoid letting the resource hide elsewhere (like in separated vars)
- FETCH_RESOURCE is intelligent enough to handle dealing with any issues this causes */
- le->refcount = 1;
- } /* FAILURE is not an option */
- zend_list_delete(Z_RESVAL_P(zhash));
+ /* Destroy the object as it should not be used further */
+ convert_to_null(zhash);
@nikic

nikic Mar 3, 2014

Owner

This looks pretty ugly. Modifying a zval that was not passed by reference is bad manners. And in the method call case, it's really weird that you do $obj->final() and suddenly $obj is null. I'd not do the conversion here and just rely on the context-initialization exceptions mentioned in a different comment to prevent further usage of this object.

@realityking

realityking Mar 3, 2014

Contributor

I agree it's pretty ugly, especially in the OO API. However that's the behavior of the procedural API and code might rely on this destroying the object/resource,

Contributor

realityking commented Apr 30, 2014

Closing in favor of #660. This way we can discuss the object oriented interface at a later time.

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