CINXE.COM

PHP: internals:review_comments

<!DOCTYPE html> <html> <head> <meta charset="utf-8"> <meta name="viewport" content="width=device-width, initial-scale=1.0"> <title>PHP: internals:review_comments</title> <meta name="generator" content="DokuWiki"/> <meta name="robots" content="index,follow"/> <meta name="keywords" content="internals,review_comments"/> <link rel="search" type="application/opensearchdescription+xml" href="/lib/exe/opensearch.php" title="PHP Wiki"/> <link rel="start" href="/"/> <link rel="contents" href="/internals/review_comments?do=index" title="Sitemap"/> <link rel="manifest" href="/lib/exe/manifest.php"/> <link rel="alternate" type="application/rss+xml" title="Recent Changes" href="/feed.php"/> <link rel="alternate" type="application/rss+xml" title="Current namespace" href="/feed.php?mode=list&amp;ns=internals"/> <link rel="alternate" type="text/html" title="Plain HTML" href="/_export/xhtml/internals/review_comments"/> <link rel="alternate" type="text/plain" title="Wiki Markup" href="/_export/raw/internals/review_comments"/> <link rel="canonical" href="https://wiki.php.net/internals/review_comments"/> <link rel="stylesheet" href="/lib/exe/css.php?t=phpnet&amp;tseed=da24444b949712713b97802ec0312cde"/> <!--[if gte IE 9]><!--> <script >/*<![CDATA[*/var NS='internals';var JSINFO = {"id":"internals:review_comments","namespace":"internals","ACT":"show","useHeadingNavigation":0,"useHeadingContent":0}; /*!]]>*/</script> <script src="/lib/exe/jquery.php?tseed=f0349b609f9b91a485af8fd8ecd4aea4" defer="defer">/*<![CDATA[*/ /*!]]>*/</script> <script src="/lib/exe/js.php?t=phpnet&amp;tseed=da24444b949712713b97802ec0312cde" defer="defer">/*<![CDATA[*/ /*!]]>*/</script> <!--<![endif]--> <link media='all' rel='stylesheet' href='//shared.php.net/styles/defaults.css?filemtime=1630005818'/> <link media='all' rel='stylesheet' href='//shared.php.net/styles/wiki.css?filemtime=1630005818'/> <link href="//fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,600,400italic,600italic|Source+Code+Pro&amp;subset=latin,latin-ext" rel="stylesheet"> <link rel="shortcut icon" href="//php.net/favicon.ico"> <!--[if lt IE 9]> <script src="//shared.php.net/js/html5shiv.js"></script> <![endif]--> </head> <body id=""> <div class="wrap"> <header class='clearfix'> <div id="mainmenu-toggle-overlay"></div> <input type="checkbox" id="mainmenu-toggle"> <nav class="fullscreen"> <div class="mainscreen"> <a href="/" class="home"><img src="//php.net/images/logo.php?" width="48" height="24" alt="php"><span class="subdomain"></span></a> <ul> <li><a href="?do=login">Login</a></li> <li><a href="?do=register">Register</a></li> </ul> </div> <div class="secondscreen"> <form method="get" action="/start"> <input type="search" placeholder="Search" name="id" class="search"> <input type="hidden" name="do" value="search"> </form> </div> </nav> </header> <div id="flash-message"></div> <nav id="megadropdown"></nav> <section class="fullscreen clearfix"> <section id="breadcrumbs"> <nav> <span class="bchead">You are here: </span><span class="home"><bdi><a href="/start" class="wikilink1" title="start" data-wiki-id="start">start</a></bdi></span> › <bdi><a href="/internals/start" class="wikilink1" title="internals:start" data-wiki-id="internals:start">internals</a></bdi> › <bdi><a href="/internals/review_comments" class="wikilink1" title="internals:review_comments" data-wiki-id="internals:review_comments">review_comments</a></bdi></nav> </section> <section class="mainscreen"> <!--[if lte IE 7 ]><div id="IE7"><![endif]--><!--[if IE 8 ]><div id="IE8"><![endif]--> <div id="dokuwiki__site"><div id="dokuwiki__top" class="site dokuwiki mode_show tpl_phpnet "> <div class="wrapper group"> <!-- ********** CONTENT ********** --> <div id="dokuwiki__content"><div class="pad group"> <div class="pageId"><span>internals:review_comments</span></div> <div class="page group"> <!-- wikipage start --> <h2 class="sectionedit1" id="common_comments_from_reviewing_pecl_proposals">Common comments from reviewing PECL proposals</h2> <div class="level2"> <p> 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. </p> <p> <strong>Note to documentation team:</strong> When adding this information to the manual please keep a reference on this page! </p> </div> <h1 class="sectionedit2" id="php_extnameh_should_be_minimal">php_$extname.h should be minimal</h1> <div class="level1"> <p> When reviewing extensions one can often see a long <em>php_$extname.h</em> file, which is bad. The <em>php_$extname.h</em> 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&#039;s module functions (like PHP_MINIT_FUNCTION), a module entry pointer and a version definition. </p> <p> If needed it may contain some public C-level APIs for other extensions. If there&#039;s a larger set of exported functionality these should get an extra header though. When providing such APIs the <em>php_$extname.h</em> still shall not include other headers. </p> <p> 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 <em>main/internal_functions.c</em> and <em>main/internal_functions_cli.c</em> which will include the <em>php_$extname.h</em> 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. </p> <p> A good starting point for a <em>php_$extname.h</em> is this: </p> <pre class="file">/* +----------------------------------------------------------------------+ | 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: | | http://www.php.net/license/3_01.txt | | 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 | | license@php.net so we can mail you a copy immediately. | +----------------------------------------------------------------------+ | Author: Ted User &lt;ted.user@php.net&gt; | +----------------------------------------------------------------------+ */ #ifndef PHP_EXTNAME_H #define PHP_EXTNAME_H extern zend_module_entry extname_module_entry; #define phpext_extname_ptr &amp;extname_module_entry #endif</pre> </div> <h1 class="sectionedit3" id="don_t_use_c99_for_portability_reasons">Don&#039;t use C99 for portability reasons</h1> <div class="level1"> <p> C99 is more than 10 years old. Still compiler vendors (most notably Microsoft Visual C) have only limited support of C99 features. Using C99 in an extension code can therefore limit the portability. C99 features include </p> <ul> <li class="level1"><div class="li"> inline functions</div> </li> <li class="level1"><div class="li"> intermingled declarations and code: variable declaration is no longer restricted to file scope or the start of a compound statement (block), similar to C++</div> </li> <li class="level1"><div class="li"> several new data types, including long long int, optional extended integer types, an explicit boolean data type, and a complex type to represent complex numbers</div> </li> <li class="level1"><div class="li"> variable-length arrays</div> </li> <li class="level1"><div class="li"> support for one-line comments beginning with .//., as in BCPL or C++</div> </li> <li class="level1"><div class="li"> new library functions, such as snprintf</div> </li> <li class="level1"><div class="li"> new header files, such as stdbool.h, complex.h, tgmath.h, and inttypes.h</div> </li> <li class="level1"><div class="li"> type-generic math functions</div> </li> <li class="level1"><div class="li"> improved support for IEEE floating point</div> </li> <li class="level1"><div class="li"> designated initializers</div> </li> <li class="level1"><div class="li"> compound literals</div> </li> <li class="level1"><div class="li"> support for variadic macros (macros with a variable number of arguments)</div> </li> <li class="level1"><div class="li"> restrict qualification allows more aggressive code optimization</div> </li> <li class="level1"><div class="li"> universal character names, which allows user variables to contain other characters than the standard character set</div> </li> </ul> <p> (Source: <a href="http://en.wikipedia.org/wiki/C99#Design" class="urlextern" title="http://en.wikipedia.org/wiki/C99#Design" rel="ugc nofollow">http://en.wikipedia.org/wiki/C99#Design</a>) </p> <p> A common mistake is the second item. Note that the macro TSRMLS_FETCH() includes a variable declaration. This macro should be the last item in the variable declaration. Code like this is invalid, according to this rule: </p> <pre class="code">static long myext_do_something() { long a, b; a = myext_get_a(); TSRMLS_FETCH(); b = myext_get_b(TSRMLS_C); long sum = a+b; return sum; }</pre> <p> This function might be written like this: </p> <pre class="code">static long myext_do_something() { long a, b; long sum; TSRMLS_FETCH(); a = myext_get_a(); b = myext_get_b(TSRMLS_C); sum = a+b; return sum; }</pre> </div> <h1 class="sectionedit4" id="don_t_use_zend_error">Don&#039;t use zend_error</h1> <div class="level1"> <p> <em>zend_error()</em> should only be used inside the engine. Inside PHP extensions only PHP&#039;s error functions shoudl be used. Typically <em>php_error_docref()</em> is the best choice. <em>php_error_docref()</em> will extend the error message by extra information, like the current function name and properly escape output where needed. <em>zend_error()</em> is used by the Zend Engine due to PHP&#039;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. </p> </div> <h1 class="sectionedit5" id="if_zend_parse_parameters_fails_do_a_simple_return">If zend_parse_parameters() fails do a simple return</h1> <div class="level1"> <p> The <em>zend_parse_parameters()</em> 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 <em>return;</em> the code also will keep the predefined value for the functions <em>return_value</em>, which is <em>NULL</em>. Returning NULL to the user is the default behaviour documented in the manual: </p> <blockquote><div class="no"> <strong>Note:</strong> 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 <strong>NULL</strong> but this is just a convention, and cannot be relied upon.</div></blockquote> <p> (Source: <a href="http://php.net/functions.internal" class="urlextern" title="http://php.net/functions.internal" rel="ugc nofollow">http://php.net/functions.internal</a>) </p> <p> Even though this is a convention this rule should be followed unless there are good reasons. </p> <p> Those are wrong: </p> <pre class="code">if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, &quot;s&quot;, &amp;arg, &amp;arg_len) == FAILURE) { RETURN_FALSE; } if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, &quot;s&quot;, &amp;arg, &amp;arg_len) == FAILURE) { php_error_docref(NULL, TSRMLS_CC, E_WARNING, &quot;Error while parsing parameters&quot;): return; }</pre> <p> This form is suggested: </p> <pre class="code">if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, &quot;s&quot;, &amp;arg, &amp;arg_len) == FAILURE) { return; }</pre> <p> See also README.PARAMETER_PARSING_<abbr title="Application Programming Interface">API</abbr> in the PHP source tree for your version of PHP. </p> </div> <h1 class="sectionedit6" id="rinitrshutdown_should_be_as_small_as_possible">RINIT/RSHUTDOWN should be as small as possible</h1> <div class="level1"> <p> RINIT/RSHUTDOWN are being called on every request handled by PHP, independently from whether the functionality is needed or not. Often it is a good alternative to delay initialisation of external libraries etc. which is commonly done in RINIT till the script access this for the first time or doing it globally in MINIT. RSHUTDOWN functionality can often be bound to resource/object destructors. </p> </div> <h1 class="sectionedit7" id="empty_rinitrshutdown_functions_should_be_removed">Empty RINIT/RSHUTDOWN functions should be removed</h1> <div class="level1"> <p> If RINIT/RSHUTDOWN functions aren&#039;t needed they should be removed and the entries of the module entry structure should be set to NULL. Besides having less code = less chance of bugs this brings a tiny performance improvement as it saves 2 indirect function calls per request, which PHP would happen to execute on every request. A module structure might look like this: </p> <pre class="code">zend_module_entry myext_module_entry = { STANDARD_MODULE_HEADER, &quot;myext&quot;, myext_functions, PHP_MINIT(myext), PHP_MSHUTDOWN(myext), NULL, NULL, PHP_MINFO(myext), PHP_MYEXT_VERSION, STANDARD_MODULE_PROPERTIES };</pre> <p> If not needed one might also remove the function table, MINIT and MSHUTDOWN if those aren&#039;t needed, but as they are only accessed during the initial PHP startup, not for every request, this is less relevant. </p> <p> Also see the previous item. </p> </div> <h1 class="sectionedit8" id="pecl_site_conformity">PECL site conformity</h1> <div class="level1"> <p> A few conformity checks are done on the tarball upload, the PECL site will check for </p> <p> . presence of LICENSE or COPYING in the package.xml, add it to the root dir with the line like this </p> <pre class="code">&lt;file name=&quot;LICENSE&quot; role=&quot;doc&quot; /&gt;</pre> <p> . for version string compliance in package.xml and the extension source code. The following should be defined in one of the extension header files </p> <pre class="code">#define PHP_EXTNAME_VERSION &quot;1.2.3&quot;</pre> <p> This macros has to be used within your extname_module_entry to indicate the extension version. The version string in both package.xml and the defined macros have to match. The work of adding that macros could be already done when using ext_skel on PHP versions as of 5.4.21 or 5.5.5. </p> </div> <!-- wikipage stop --> </div> <div class="docInfo"><bdi>internals/review_comments.txt</bdi> · Last modified: 2017/09/22 13:28 by <bdi>127.0.0.1</bdi></div> </div></div><!-- /content --> <hr class="a11y" /> <!-- PAGE ACTIONS --> <div id="dokuwiki__pagetools"> <h3 class="a11y">Page Tools</h3> <div class="tools"> <ul> <li><a href="/internals/review_comments?do=edit" class="action source" accesskey="v" rel="nofollow" title="Show pagesource [V]"><span>Show pagesource</span></a></li><li><a href="/internals/review_comments?do=revisions" class="action revs" accesskey="o" rel="nofollow" title="Old revisions [O]"><span>Old revisions</span></a></li><li><a href="/internals/review_comments?do=backlink" class="action backlink" rel="nofollow" title="Backlinks"><span>Backlinks</span></a></li><li><a href="#dokuwiki__top" class="action top" accesskey="t" rel="nofollow" title="Back to top [T]"><span>Back to top</span></a></li> </ul> </div> </div> </div><!-- /wrapper --> </div></div><!-- /site --> <div class="no"><img src="/lib/exe/taskrunner.php?id=internals%3Areview_comments&amp;1740547320" width="2" height="1" alt="" /></div> <div id="screen__mode" class="no"></div> <!--[if ( lte IE 7 | IE 8 ) ]></div><![endif]--> </section> <section class="secondscreen"> <!-- TOC START --> <div id="dw__toc" class="dw__toc"> <h3 class="toggle">Table of Contents</h3> <div> <ul class="toc"> <li class="level2"><div class="li"><a href="#common_comments_from_reviewing_pecl_proposals">Common comments from reviewing PECL proposals</a></div></li> <li class="level1"><div class="li"><a href="#php_extnameh_should_be_minimal">php_$extname.h should be minimal</a></div></li> <li class="level1"><div class="li"><a href="#don_t_use_c99_for_portability_reasons">Don&#039;t use C99 for portability reasons</a></div></li> <li class="level1"><div class="li"><a href="#don_t_use_zend_error">Don&#039;t use zend_error</a></div></li> <li class="level1"><div class="li"><a href="#if_zend_parse_parameters_fails_do_a_simple_return">If zend_parse_parameters() fails do a simple return</a></div></li> <li class="level1"><div class="li"><a href="#rinitrshutdown_should_be_as_small_as_possible">RINIT/RSHUTDOWN should be as small as possible</a></div></li> <li class="level1"><div class="li"><a href="#empty_rinitrshutdown_functions_should_be_removed">Empty RINIT/RSHUTDOWN functions should be removed</a></div></li> <li class="level1"><div class="li"><a href="#pecl_site_conformity">PECL site conformity</a></div></li> </ul> </div> </div> <!-- TOC END --> </section> </section><!-- .fullscreen --> <footer> <nav class="fullscreen"> <ul> <li><a href="//php.net/copyright">Copyright © 2001-2025 The PHP Group</a></li> <li><a href="//php.net/sites">Other PHP.net sites</a></li> <li><a href="//php.net/privacy">Privacy policy</a></li> </ul> </nav> </footer> </div><!-- .wrap --> </body> </html>

Pages: 1 2 3 4 5 6 7 8 9 10