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.

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

Fix a PHP 8 deprecation warning #45

Merged
merged 2 commits into from
Nov 19, 2020
Merged

Conversation

angrybrad
Copy link
Contributor

@angrybrad angrybrad mentioned this pull request Oct 22, 2020
Copy link
Owner

@darylldoyle darylldoyle left a comment

Choose a reason for hiding this comment

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

Hi @angrybrad, thanks for the PR, it's much appreciated!

Looking at the changes that have been made, do you think it's a better idea to target the PHP version or the libxml version?

I'm thinking libxml as that will also catch anyone that compiles PHP8 with libxml < 2.9.0. What are your thoughts there?

@angrybrad
Copy link
Contributor Author

@darylldoyle good question. Looks like they recommend either way: https://php.watch/versions/8.0#libxml_disable_entity_loader-deprecation

Not sure how many people would compile PHP with an older version of libxml than 2.9.0, but happy to update to this instead:

if (\LIBXML_VERSION < 20900) {
    libxml_disable_entity_loader(true);
}

@darylldoyle
Copy link
Owner

Thanks, @angrybrad! I think I'd feel more comfortable targeting the libxml version directly, rather than the PHP version. If you don't mind making that change when you get time, I'll be happy to merge this in 🙂

@angrybrad
Copy link
Contributor Author

@darylldoyle done!

@darylldoyle darylldoyle merged commit 88eaa33 into darylldoyle:master Nov 19, 2020
@darylldoyle
Copy link
Owner

Thanks @angrybrad that's great. I appreciate you taking the time to make this PR. I'll get an update for a new version out shortly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants