This is an old revision of the document!

While reviewing PECL proposals there are some common issues which can be observed. This page collects some of those to provide extended information and saving reviewers from explaining those multiple times. This is different from other sections on this wiki a it focusses on review perspective.

Note to documentation team: When adding this information to the manual please keep a reference on this page!

php_$extname.h should be minimal

When reviewing extensions one can often see a long php_$extname.h file, which is bad. The php_$extname.h header should only contain a limited set of declarations. It should not include library or system headers. At most, it should contain declarations for the extension's module functions (like PHP_MINIT_FUNCTION), a module entry pointer and a version definition.

If needed it may contain some public C-level APIs for other extensions. If there's a larger set of exported functionality these should get an extra header though. When providing such APIs the php_$extname.h still shall not include other headers.

The reason for this is that for a static build of the extension as part of the main PHP build the PHP configure script will create main/internal_functions.c and main/internal_functions_cli.c which will include the php_$extname.h headers of all activated extensions for creating the extension startup structure. Including too many declarations there not only increases compile times but also increases the possibility of conflicts between declarations as different extensions might use the same sourcelevel symbols for different purpose.

A good starting point for a php_$extname.h is this:

  | PHP Version 5                                                        |
  | Copyright (c) 1997-2012 The PHP Group                                |
  | This source file is subject to version 3.01 of the PHP license,      |
  | that is bundled with this package in the file LICENSE, and is        |
  | available through the world-wide-web at the following url:           |
  |                                  |
  | If you did not receive a copy of the PHP license and are unable to   |
  | obtain it through the world-wide-web, please send a note to          |
  | so we can mail you a copy immediately.               |
  | Author: Ted User <>                                  |


extern zend_module_entry extname_module_entry;
#define phpext_extname_ptr &extname_module_entry


Don't use zend_error

zend_error() should only be used inside the engine. Inside PHP extensions only PHP's error functions shoudl be used. Typically php_error_docref() is the best choice. php_error_docref() will extend the error message by extra information, like the current function name and properly escape output where needed. zend_error() is used by the Zend Engine due to PHP's modular architecture where the Zend Engine and TSRM should be compilable without other parts of PHP. The engine therefore can not use PHP level APIs. This limitation does not exist in extensions.

If zend_parse_parameters() fails do a simple return

The zend_parse_parameters() function is used to access the function parameters from the executors stack. It will automatically provide an error message to the user. By doing a plain return; the code also will keep the predefined value for the functions return_value, which is NULL. Returning NULL to the user is the default behaviour documented in the manual:

Note: If the parameters given to a function are not what it expects, such as passing an array where a string is expected, the return value of the function is undefined. In this case it will likely return NULL but this is just a convention, and cannot be relied upon.


Even though this is a convention this rule should be followed unless there are good reasons.

Those are wrong:

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &arg, &arg_len) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &arg, &arg_len) == FAILURE) {
  php_error_docref(NULL, TSRMLS_CC, E_WARNING, "Error while parsing parameters"):

This form is suggested:

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &arg, &arg_len) == FAILURE) {

See also README.PARAMETER_PARSING_API in the PHP source tree for your version of PHP.

internals/review_comments.1357257705.txt.gz · Last modified: 2017/09/22 13:28 (external edit)