Skip to content

Conversation

@Girgias
Copy link
Member

@Girgias Girgias commented Oct 3, 2025

This has uncovered a memory leak, which I'm not exactly sure how it needs to be fixed.

These tests are in preparation for some refactoring I was doing and realised there were insufficient tests.

A thing, that probably requires an RFC that would be a good idea is to mandate that the class name passed to stream_filter_register actually exists and is a child of php_user_filter

@Girgias Girgias marked this pull request as ready for review October 3, 2025 12:40
@Girgias Girgias requested a review from bukka as a code owner October 3, 2025 12:40
@ndossche
Copy link
Member

ndossche commented Oct 4, 2025

Leak fix here: #20058

@Girgias Girgias requested a review from ndossche November 14, 2025 13:45
Copy link
Member

@ndossche ndossche left a comment

Choose a reason for hiding this comment

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

Looks fine, I wonder whether it would be wise to add #[AllowDynamicProperties] to silence some deprecations warnings in some of these tests. We would need to add them anyway later when we reach PHP9 (or have a different solution than adding dynamic props by streams code)

@Girgias
Copy link
Member Author

Girgias commented Nov 16, 2025

Looks fine, I wonder whether it would be wise to add #[AllowDynamicProperties] to silence some deprecations warnings in some of these tests. We would need to add them anyway later when we reach PHP9 (or have a different solution than adding dynamic props by streams code)

The general expectation is that a stream filter extends the built-in php_user_filter, which I think we should enforce and that class defines the properties.

@Girgias Girgias merged commit 9c33091 into php:master Nov 16, 2025
9 checks passed
@Girgias Girgias deleted the stream-filter-tests branch November 16, 2025 14:20
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.

2 participants