Skip to content

Fix #79884: PHP_CONFIG_FILE_PATH is meaningless#5884

Closed
cmb69 wants to merge 1 commit intophp:PHP-7.3from
cmb69:cmb/79884
Closed

Fix #79884: PHP_CONFIG_FILE_PATH is meaningless#5884
cmb69 wants to merge 1 commit intophp:PHP-7.3from
cmb69:cmb/79884

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