Skip to content

Fix phar/tests/bug77432.phpt #13129

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 phar/tests/bug77432.phpt #13129

wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

  • For Windows we just have to set the right error_reporting value
  • Test cannot be used repeatedly on Opcache because the unlink will have no effect because of caching.

@nielsdos
Fix phar/tests/bug77432.phpt
53252c3 
- For Windows we just have to set the right error_reporting value
- Test cannot be used repeatedly on Opcache because the unlink will have
  no effect because of caching.
@nielsdos nielsdos requested a review from iluuu1994 January 12, 2024 07:59
iluuu1994
iluuu1994 approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Could we not just force-disable opcache.file_cache? But it's not really very important.

@nielsdos
Copy link
Member Author

Could we not just force-disable opcache.file_cache? But it's not really very important.

The error you linked did not seem to involve the file_cache though, rather the error was in the shm workflow?

@nielsdos nielsdos closed this in 5a988d5 Jan 12, 2024
@iluuu1994
Copy link
Member

iluuu1994 commented Jan 12, 2024 •
edited
Loading

@nielsdos Variation tests file-cache, mostly. The test failed in the "Test File Cache (prime shm, use shm)". It passes the --file-cache-use flag to run-tests.php, which means it sets opcache.validate_timestamps=0. I would expect this to skip the inclusion of the phar file and load it from cache instead. Maybe I'm mistaken.

Anyway, skipping opcache for this test is fine.

@nielsdos
Copy link
Member Author

Ah okay I see, reasoning makes sense.
And yeah, this should suffice :)

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

@iluuu1994 iluuu1994 iluuu1994 approved these changes

Assignees
No one assigned
Labels
Extension: phar
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nielsdos @iluuu1994