Skip to content

Fix #79884: PHP_CONFIG_FILE_PATH is meaningless #5884

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix #79884: PHP_CONFIG_FILE_PATH is meaningless #5884

wants to merge 1 commit into from
+1 −1

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jul 22, 2020

It does not make sense to make assumptions about PHP_CONFIG_FILE_PATH
during build time, since that value is never used during run time on
Windows. Since there is no --with-config-file-path on Windows
either, we define PHP_CONFIG_FILE_PATH as "".

@cmb69
Fix #79884: PHP_CONFIG_FILE_PATH is meaningless
1971aae 
It does not make sense to make assumptions about `PHP_CONFIG_FILE_PATH`
during build time, since that value is never used during run time on
Windows.  Since there is no `--with-config-file-path` on Windows
either, we define `PHP_CONFIG_FILE_PATH` as `""`.
@cmb69
Copy link
Member Author

cmb69 commented Jul 22, 2020

The AppVeyor test fail looks unrelated.

@nikic
Copy link
Member

nikic commented Jul 22, 2020

One oddity I see here is that we export PHP_CONFIG_FILE_PATH as a PHP-level constant. I wonder if that shouldn't be reporting the same path that gets determined in php_init_config? Not that the constant seems particularly useful...

@cmb69
Copy link
Member Author

cmb69 commented Jul 22, 2020

I agree that the php_ini_search_path is more useful information, but I'm not sure whether it's a good idea to expose it as PHP_CONFIG_FILE_PATH – might better be a new constant or function, even for PHP 8. I don't have a strong opinion on that, though.

nikic
nikic approved these changes Jul 23, 2020
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with this for now.

@cmb69
Copy link
Member Author

cmb69 commented Jul 23, 2020

Thanks! Applied as 15efb96.

@cmb69 cmb69 closed this Jul 23, 2020
@cmb69 cmb69 deleted the cmb/79884 branch July 23, 2020 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cmb69 @nikic @carusogabriel