Skip to content
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

ext/libxml: Fixed custom external entity loader returning an invalid resource leading to a confusing TypeError message #18096

Open
wants to merge 1 commit into
base: PHP-8.3
Choose a base branch
from
Open

ext/libxml: Fixed custom external entity loader returning an invalid resource leading to a confusing TypeError message #18096

wants to merge 1 commit into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Mar 17, 2025

Split from #18079

@Girgias Girgias requested a review from nielsdos as a code owner March 17, 2025 14:12
nielsdos
nielsdos reviewed Mar 17, 2025
View reviewed changes
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Now I'm confused why the old code didn't emit the warnings but only outputted the exception...
I also am a bit skeptical about the relaxing from exception to warning, I think it should stay an exception just with the better wording

@Girgias
Copy link
Member Author

Girgias commented Mar 18, 2025

Now I'm confused why the old code didn't emit the warnings but only outputted the exception... I also am a bit skeptical about the relaxing from exception to warning, I think it should stay an exception just with the better wording

It used to be a warning in PHP 7, but I'm happy to improve the error message for this. Any suggestions?

@Girgias
ext/libxml: Fixed custom external entity loader returning an invalid …
18d554f 
…resource leading to a confusing TypeError message
@Girgias Girgias force-pushed the libxml-entity-loader-fix branch from f4f3b30 to 18d554f Compare March 21, 2025 21:03
@Girgias Girgias requested a review from nielsdos March 21, 2025 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsdos nielsdos nielsdos approved these changes

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

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