[NFR] Allow Numeric Keys in Phalcon\Config

[ghost]

See Issue #696

[ghost]

See also #731 and #732 — this pull request is also affected by those issues.

[phalcon]

I think is simpler check if the array only contains numeric indexes keeping the current behavior:

int phalcon_hash_string_keys(zval *data)
{   
    int is_numeric = 1;
    zval **data, **tmp;
    char *string_key;
    uint string_len;
    ulong num_key;

    if (Z_TYPE_P(data) == IS_ARRAY) {

        zend_hash_internal_pointer_reset(Z_ARRVAL_P(data));

        while (zend_hash_get_current_data(Z_ARRVAL_P(data), (void **) &tmp) == SUCCESS) {
            if (zend_hash_get_current_key_ex(Z_ARRVAL_P(data), &string_key, &string_len, &num_key, 0, NULL) == HASH_KEY_IS_STRING) {
                is_numeric = 0;
                                break;
            }
            zend_hash_move_forward(Z_ARRVAL_P(data));
        }

    }

    return is_numeric;
}

Then use this function to check if the hash is numeric, instead of doing this: https://github.com/phalcon/cphalcon/blob/1.2.0/ext/config.c#L111

[ghost]

OK, but what if we have a mix of string and numeric keys?

$x = new Phalcon\Config(array('x' => array('a' => array(...), 1 => array(...))));

• if we treat such an array as array (ie, do not wrap it with Phalcon\Config), we lose the ability to use $x->x->a->property syntax;
• if we wrap it with Phalcon\Config, then we will choke on $x->x[1] because all properties are expected to be strings :-)

[ghost]

Or what if drop arrays all together and always use Phalcon\Config for any types of arrays? The advantage is that we won't have to scan the array to check its type.

[phalcon]

I see arrays as endpoints for configurations, why would you want to name a configuration 1, 256, 121921? instead of a readable valid string that has a useful meaning?

<?php

$enviroments = new Phalcon\Config([
    'development' => array(
        'hosts' => 'my.database.com',
        'ports' => array(3306, 3307)
    )
]);

[ghost]

Well, personally I agree with you but please read #696 — use of constants as array keys (like CURLOPT_HEADER) seems to be a valid argument.

[phalcon]

Yes, we can scan the array, normally arrays in configurations aren't so big and the overhead is minimal, in most cases at the first iteration it will find a string key, processing the array as normal, the thing is, if string/numeric keys are mixed you'll got an exception or something.

[ghost]

So just to summarize:

• if the array is all numeric, we keep it as array()
• if the array is all string, we convert it to Phalcon\Config
• if array is mixed, we declare this as undefined behavior

Is this correct?

if so, we can simplify this to

@@ -108,7 +108,7 @@ 
       PHALCON_GET_HVALUE(value);

       if (Z_TYPE_P(value) == IS_ARRAY) { 
-        if (!phalcon_array_isset_long(value, 0)) {
+        if (zend_hash_get_current_key_type(Z_ARRVAL_P(value)) != HASH_KEY_IS_LONG) {
           PHALCON_INIT_NVAR(config_value);
           object_init_ex(config_value, phalcon_config_ce);
           phalcon_call_method_p1_noret(config_value, "__construct", value);

What do you think?

[phalcon]

More like this:

//Throw exceptions on unexpected parameters
if (Z_TYPE_P(array_config) != IS_ARRAY) { 
    if (Z_TYPE_P(array_config) != IS_NULL) {
        PHALCON_THROW_EXCEPTION_STR(phalcon_config_exception_ce, "The configuration must be an Array");
        return;
    } else {
        RETURN_MM_NULL;
    }
}

phalcon_is_iterable(array_config, &ah0, &hp0, 0, 0);

while (zend_hash_get_current_data_ex(ah0, (void**) &hd, &hp0) == SUCCESS) {

    PHALCON_GET_HKEY(key, ah0, hp0);
    PHALCON_GET_HVALUE(value);

    //First level array passed to the function
    if (Z_TYPE_P(key) != IS_STRING) { 
        PHALCON_THROW_EXCEPTION_STR(phalcon_config_exception_ce, "Numeric indexes aren't allowed");
        return;
    }

    //Handle sub-arrays
    if (Z_TYPE_P(value) == IS_ARRAY) { 

        //Check that all keys in 'value' are strings
        if (phalcon_hash_string_keys(value)) {

            PHALCON_INIT_NVAR(config_value);
            object_init_ex(config_value, phalcon_config_ce);
            phalcon_call_method_p1_noret(config_value, "__construct", value);

            phalcon_update_property_zval_zval(this_ptr, key, config_value TSRMLS_CC);

        } else {
            //Add it as array
            phalcon_update_property_zval_zval(this_ptr, key, value TSRMLS_CC);
        }
    } else {
        phalcon_update_property_zval_zval(this_ptr, key, value TSRMLS_CC);
    }

    zend_hash_move_forward_ex(ah0, &hp0);
}

[ghost]

OK, I leave it to you then :-)

[ghost]

Hmm, please take a look at #731 — looks like arrays with mixed indices could be troublesome :-(

[iforp]

Why you don't want to implement as I said before?
(with checking of first key type and Phalcon\Config\RawArray to use exactly array) #696 (comment)

It is low overhead and ability to set array even with only string keys

[ghost]

if (Z_TYPE_P(array_config) != IS_ARRAY) { 
    if (Z_TYPE_P(array_config) != IS_NULL) {
        PHALCON_THROW_EXCEPTION_STR(phalcon_config_exception_ce, "The configuration must be an Array");
        return;
    } else {
        RETURN_MM_NULL;
    }
}

can be optimized to

if (Z_TYPE_P(array_config) == IS_NULL) {
    RETURN_MM_NULL;
}

provided that

 ZEND_BEGIN_ARG_INFO_EX(arginfo_phalcon_config___construct, 0, 0, 0)
-   ZEND_ARG_INFO(0, arrayConfig)
+   ZEND_ARG_ARRAY_INFO(0, arrayConfig, 1)
 ZEND_END_ARG_INFO()

[phalcon]

Would it throw a phalcon_config_exception_ce exception?

[ghost]

No, if this is the goal then it won't work.

Any attempt to pass anything other than array/NULL will result in a Catchable fatal error like Argument 1 passed to Phalcon\Config() must be of the type array, XXX given

Catchable fatals can ony be caught by the error handler (which can rethrow them as exceptions) so probably this is a bad idea.