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

Voidify some Zend APIs #5805

Closed
wants to merge 9 commits into from
Closed

Voidify some Zend APIs #5805

wants to merge 9 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jul 3, 2020

These always return SUCCESS and therefore the result is meaningless.

I didn't act on the add_next_index_*() functions as they return zend_hash_next_index_insert(...) but from my understanding this function should also always return SUCCESS.

@Girgias
Copy link
Member Author

Girgias commented Jul 7, 2020

@nikic can I merge this, and I imagine this needs an entry in UPGRADING.INTERNALS correct?

@php-pulls php-pulls closed this in 9839752 Jul 9, 2020
@Girgias Girgias deleted the voidify-api branch July 9, 2020 12:18
@Jan-E
Copy link
Contributor

Jan-E commented Jul 21, 2020

This PR & commits caused a fatal error when compiling the sqlsrv and pdo_sqlsrv extensions under PHP 8.
Can you document the change in UPGRADING_INTERNALS?

https://github.com/microsoft/msphpsql/blob/master/source/shared/core_sqlsrv.h#L2540-L2546

    inline void sqlsrv_add_assoc_zval( _Inout_ sqlsrv_context& ctx, _Inout_ zval* array_z, _In_ const char* key, _In_ zval* val TSRMLS_DC )
    {
        int zr = ::add_assoc_zval(array_z, key, val);
        CHECK_ZEND_ERROR (zr, ctx, SQLSRV_ERROR_ZEND_HASH ) {
            throw CoreException();
        }
    }

@Girgias
Copy link
Member Author

Girgias commented Jul 21, 2020

This PR & commits caused a fatal error when compiling the sqlsrv and pdo_sqlsrv extensions under PHP 8.
Can you document the change in UPGRADING_INTERNALS?

https://github.com/microsoft/msphpsql/blob/master/source/shared/core_sqlsrv.h#L2540-L2546

    inline void sqlsrv_add_assoc_zval( _Inout_ sqlsrv_context& ctx, _Inout_ zval* array_z, _In_ const char* key, _In_ zval* val TSRMLS_DC )
    {
        int zr = ::add_assoc_zval(array_z, key, val);
        CHECK_ZEND_ERROR (zr, ctx, SQLSRV_ERROR_ZEND_HASH ) {
            throw CoreException();
        }
    }

The add_assoc_zval call can't fail anymore so it's just a matter of dropping the check.
This is already documented in UPGRADING_INTERNALS under section t. 1. first dash

@Jan-E
Copy link
Contributor

Jan-E commented Jul 21, 2020

Forget the 'please document it'. I see that you already did that.

@Jan-E
Copy link
Contributor

Jan-E commented Jul 21, 2020

OK, so the new code for PHP 8 should be

    inline void sqlsrv_add_assoc_zval( _Inout_ sqlsrv_context& ctx, _Inout_ zval* array_z, _In_ const char* key, _In_ zval* val TSRMLS_DC )
    {
        add_assoc_zval(array_z, key, val);
    }

@Girgias
Copy link
Member Author

Girgias commented Jul 21, 2020

OK, so the new code for PHP 8 should be

    inline void sqlsrv_add_assoc_zval( _Inout_ sqlsrv_context& ctx, _Inout_ zval* array_z, _In_ const char* key, _In_ zval* val TSRMLS_DC )
    {
        add_assoc_zval(array_z, key, val);
    }

That's correct :)

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.

3 participants