[NFR] Explicitly mention dependecies on modules used at coompile time #840

Merged
merged 8 commits into from Jul 15, 2013

Projects

None yet

2 participants

@ghost
ghost commented Jul 13, 2013

Right now Phalcon uses SPL, PCRE, JSON, Session modules if they present at compile time. Those modules can be built either statically into PHP or as DSOs.

If a module Phalcon depends on is built as a DSO and then later is disabled in php.ini, this may lead to very interesting crashes due to unresolved external symbol. Module dependencies will ensure that all required modules are present at runtime and provide meaningful diagnostics if one of the required modules is missing.

This PR implements the following:

  • 0ca55d6 and 4db3834 add known module dependencies to config.m4 (strictly speaking this is necessary only when Phalcon is built statically into PHP (as an offtopic, if Phalcon is built into PHP and LTO is enabled, this could give quite noticeable performance improvement due to better opportunities for inlining and IPO)) and phalcon.c; this will ensure the correct order of initialization of extensions
  • 4db3834 also removes conditional checks against old ZEND_MODULE_API_NO numbers (those numbers correspond to PHP 4.x, if I am not mistaken) and adds GINIT function used to initialize module globals (this is the preferred way over ZEND_INIT_MODULE_GLOBALS(phalcon, php_phalcon_init_globals, NULL) used for backwards compatibility with ancient PHP versions)
  • f2392c9 introduces kernel/session.c and kernel/session.h which provide transparent access to the PHP Session API (either via calls to native functions or via calls to the userland); f4d1ee6 uses this API in ext/session/adapter.c (this looks better than a bunch of #ifdefs)
  • bce8607 provides an abstraction for preg_match regardless whether PCRE is available at compile time and now it is sufficient to call phalcon_preg_match without any #ifdefs

To be done:

  • 0ca55d6 works reliably only for *nix; Windows builds currently use a hack (in the bottom of php_phalcon.h) because I don't know the equivalent for AC_CHECK_HEADER() for config.w32; in other words, nothing has changed for Windows. I need help here.
  • build/*/config.m4 will need to be updated, too
@ghost Unknown commented on the diff Jul 13, 2013
ext/config.m4
@@ -337,5 +338,82 @@ mvc/view/engine/volt/scanner.c \
annotations/parser.c \
annotations/scanner.c"
- PHP_NEW_EXTENSION(phalcon, $phalcon_sources, $ext_shared)
+ PHP_NEW_EXTENSION(phalcon, $phalcon_sources, $ext_shared)
+ PHP_ADD_EXTENSION_DEP([phalcon], [spl])
+
+ old_CPPFLAGS=$CPPFLAGS
+ CPPFLAGS="$CPPFLAGS $INCLUDES"
+
@ghost
ghost Jul 13, 2013

PHP does not add its include paths to CFLAGS at this point, we need to do that explicitly and then restore the original value after all our checks

@ghost Unknown commented on the diff Jul 13, 2013
ext/config.m4
+ AC_CHECK_DECL(
+ [HAVE_BUNDLED_PCRE],
+ [
+ AC_CHECK_HEADERS(
+ [ext/pcre/php_pcre.h],
+ [
+ PHP_ADD_EXTENSION_DEP([phalcon], [pcre])
+ AC_DEFINE([PHALCON_USE_PHP_PCRE], [1], [Whether PHP pcre extension is present at compile time])
+ ],
+ ,
+ [[#include "main/php.h"]]
+ )
+ ],
+ ,
+ [[#include "php_config.h"]]
+ )
@ghost
ghost Jul 13, 2013

These tests check for both the declared symbol in php_config.h and presence of the required header file.
Though it is very unlikely that the header won't be there, this will make sure that the compilation won't be aborted if the header is not found.

@ghost Unknown commented on the diff Jul 13, 2013
ext/config.m4
- PHP_NEW_EXTENSION(phalcon, $phalcon_sources, $ext_shared)
+ PHP_NEW_EXTENSION(phalcon, $phalcon_sources, $ext_shared)
+ PHP_ADD_EXTENSION_DEP([phalcon], [spl])
+
+ old_CPPFLAGS=$CPPFLAGS
+ CPPFLAGS="$CPPFLAGS $INCLUDES"
+
+ AC_CHECK_HEADERS(
+ [ext/filter/php_filter.h],
+ [
+ PHP_ADD_EXTENSION_DEP([phalcon], [filter])
+ AC_DEFINE([PHALCON_USE_PHP_FILTER], [1], [Whether PHP filter extension is present at compile time])
+ ],
+ ,
+ [[#include "main/php.h"]]
+ )
@ghost
ghost Jul 13, 2013

filter extension does not leave any traces in php_config.h yet can be disabled with --disable-filter (this will be very untypical though). The only reliable way to check whether we can use the native filter API is to check for the header.

@ghost Unknown commented on the diff Jul 13, 2013
ext/db/adapter/pdo/mysql.c
phalcon_preg_match(pos, size_pattern, column_type, matches TSRMLS_CC);
- #else
- phalcon_call_func_p3(pos, "preg_match", size_pattern, column_type, matches);
- #endif
@ghost
ghost Jul 13, 2013

If I get it right, Z_SET_ISREF_P(matches)/Z_UNSET_ISREF_P(matches) are only needed when we call the userspace version.

If so, we can move these macros to phalcon_preg_match() — which will be more friendly to a developer if they forgot to surround the calls with Z_(UN)SET_ISREF_P macros.

@ghost Unknown commented on the diff Jul 13, 2013
ext/phalcon.c
@@ -363,9 +366,6 @@
return FAILURE;
}
- /** Init globals */
- ZEND_INIT_MODULE_GLOBALS(phalcon, php_phalcon_init_globals, NULL);
-
@ghost
ghost Jul 13, 2013

GINIT_FUNCTION is the preferred way over ZEND_INIT_MODULE_GLOBALS

@ghost Unknown commented on the diff Jul 13, 2013
ext/phalcon.c
@@ -715,21 +715,57 @@
return SUCCESS;
}
-zend_module_entry phalcon_module_entry = {
-#if ZEND_MODULE_API_NO >= 20010901
- STANDARD_MODULE_HEADER,
+static PHP_MINFO_FUNCTION(phalcon)
@ghost
ghost Jul 13, 2013

Sample module info function to add mentions of Phalcon to phpinfo() output.

@ghost Unknown commented on the diff Jul 13, 2013
ext/php_phalcon.h
@@ -164,3 +164,25 @@
#define UNREACHABLE() assert(0)
#define ASSUME(x) assert(!!(x));
#endif
+
+#ifndef ZEND_MOD_END
+#define ZEND_MOD_END { NULL, NULL, NULL, 0 }
+#endif
+
+/* This is a temporary fix until config.w32 is updated */
@ghost
ghost Jul 13, 2013

The hack for Windows I mentioned. This is to make sure that everything will work on Windows as it used to

@niden niden merged commit 17ef290 into phalcon:1.2.1 Jul 15, 2013
@ghost ghost deleted the unknown repository branch Jul 16, 2013
@tembem tembem referenced this pull request in phalcon/blog Aug 2, 2016
Closed

Remove Indonesian translation #60

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