Skip to content

Don't include system headers in php_config.h #17124

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Dec 12, 2024

Configuration headers (config.h, etc.), as their name implies, are supposed to contain macro definitions to avoid having them to pass on the command line[1]. It seems fine if they include other configuration headers when needed. In any way, they are supposed to be included very early in compilation units which require them.

Doing more than defining simple macros, possibly guarded by defines set during compile time (such as a compiler or platform identification), can lead to subtle issues, which may go unnoticed such as the fallback definitions in the hard-coded (!) fpm_config.h, which may actually be used unconditionally, because the appropriate system headers have not
already been included.

[1] https://www.gnu.org/software/autoconf/manual/autoconf-2.72/html_node/Configuration-Headers.html


So far this is only a partial clean-up of the different configuration files; particularly, almost all of fpm_config.h should be moved elsewhere (likely fpm.h). Before I do this, I'm asking for consensus on the approach outlined above.

Note that this PR is in no way supposed to clean up the module mess we have, nor the include mess, e.g.

#include "php.h"
#include "php_globals.h"
#include "php_variables.h"
#include "php_ini_builder.h"
#include "zend_modules.h"
#include "php.h"
#include "zend_ini_scanner.h"
#include "zend_globals.h"
#include "zend_stream.h"
#include "SAPI.h"
#include <stdio.h>
#include "php.h"

Includes php.h three times in not even 15 lines of code.

Configuration headers (config.h, etc.), as their name implies, are
supposed to contain macro definitions to avoid having them to pass
on the command line[1].  It seems fine if they include other
configuration headers when needed.  In any way, they are supposed to be
included very early in compilation units which require them.

Doing more than defining simple macros, possibly guarded by defines set
during compile time (such as a compiler or platform identification),
can lead to subtle issues, which may go unnoticed such as the fallback
definitions in the hard-coded (!) fpm_config.h, which may actually be
used unconditionally, because the appropriate system headers have not
already been included.

[1] <https://www.gnu.org/software/autoconf/manual/autoconf-2.72/html_node/Configuration-Headers.html>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant