Skip to content

[Process] Fix the removal of host-specific configuration when managing the ini settings in PhpSubprocess #58195

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
Merged
merged 1 commit into from
Sep 11, 2024
Merged

[Process] Fix the removal of host-specific configuration when managing the ini settings in PhpSubprocess #58195

merged 1 commit into from
Sep 11, 2024

Conversation

M-arcus
Copy link
Contributor

@M-arcus M-arcus commented Sep 6, 2024 •
edited by nicolas-grekas
Loading

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues
License MIT

preg_match can return the offset needed for substr in the next line, but without the flag it doesn't put the offset of the capturing group into $matches[0][1]

With the example php.ini the code breaks on execution

opcache.validate_timestamps = 0
[HOST=my.test.host]
opcache.validate_timestamps = 1

@carsonbot carsonbot added this to the 7.1 milestone Sep 6, 2024
@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.1" but it seems your PR description refers to branch "5.4, 6.4, and 7.1 for bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@M-arcus M-arcus mentioned this pull request Sep 6, 2024
Closed
@stof stof changed the title PhpSubprocess: Add flag PREG_OFFSET_CAPTURE to preg_match to identify the offset Fix the removal of host-specific configuration when managing the ini settings in PhpSubprocess Sep 6, 2024
@stof
Copy link
Member

stof commented Sep 6, 2024

It would be great to have a test covering that case to prevent regressions.

@M-arcus
Copy link
Contributor Author

M-arcus commented Sep 6, 2024

The change is inside a private function and is based on the configuration of the environment PHPUnit is executed in.

I know some ways on how to create this test, but what would be the correct Symfony way to do this? Are there other similar tests I can adapt from?

@stof
Copy link
Member

stof commented Sep 6, 2024

Well, maybe it involve a test running a separate PHP process (passing a specific php.ini so that it does not depend on the config of the host running PHPUnit) using a PHP script using that PhpSubprocess class. That's already the way other PhpSubprocess tests are written.

@OskarStark OskarStark changed the title Fix the removal of host-specific configuration when managing the ini settings in PhpSubprocess Fix the removal of host-specific configuration when managing the ini settings in PhpSubprocess Sep 10, 2024
@carsonbot carsonbot changed the title Fix the removal of host-specific configuration when managing the ini settings in PhpSubprocess [Process] Fix the removal of host-specific configuration when managing the ini settings in PhpSubprocess Sep 11, 2024
@nicolas-grekas nicolas-grekas modified the milestones: 7.1, 5.4, 6.4 Sep 11, 2024
@nicolas-grekas nicolas-grekas changed the base branch from 7.1 to 6.4 September 11, 2024 07:50
@nicolas-grekas
Copy link
Member

Thank you @M-arcus.

@nicolas-grekas nicolas-grekas merged commit f2e8474 into symfony:6.4 Sep 11, 2024
6 of 10 checks passed
@nicolas-grekas
Copy link
Member

Please come back with a follow up PR to add tests if you can.

This was referenced Sep 21, 2024
Merged
Merged
@M-arcus M-arcus deleted the patch-2 branch January 21, 2025 20:17
@richard67 richard67 mentioned this pull request Feb 1, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyrixx lyrixx Awaiting requested review from lyrixx

@yceruto yceruto Awaiting requested review from yceruto

@welcoMattic welcoMattic Awaiting requested review from welcoMattic

@kbond kbond Awaiting requested review from kbond

@chalasr chalasr Awaiting requested review from chalasr

@dunglas dunglas Awaiting requested review from dunglas

@OskarStark OskarStark Awaiting requested review from OskarStark

@jderusse jderusse Awaiting requested review from jderusse

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Bug Process Status: Needs Review
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
4 participants
@M-arcus @carsonbot @stof @nicolas-grekas