search

laminas / laminas-xml

Utility library for XML usage, best practices, and security in PHP
BSD 3-Clause "New" or "Revised" License
14 stars 8 forks source link

Add a small PHP 8.0 fix #3

Closed xvilo closed 4 years ago

xvilo commented 4 years ago
Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

I'm updating logic so that it does not get used on PHP 8.0+. In PHP 8.0 and later, PHP uses libxml versions from 2.9.0, which disabled XXE by default. libxml_disable_entity_loader() is now deprecated.

The current code will throw deprecation warnings/errors and break the code. As for as far as I understand it, it the same logic as it is now disabled by default so changing this value is not needed. And also reverting the change back to its original value.

I have opted to wrap the function calls in an if statement that checks if the current PHP version is lower than PHP 8.0. This was the easiest way I could think of to update this code without touching too much of its current behavior.

I'm not entirely sure if this is (already) wanted, this has been tested on the latest PHP 8.0 release candidate as of today.

Also, please do let me know if you have feedback!

EDIT:

Please note that I did not check any other part of the code, I just noticed this when running a project on PHP 8 as a test and came across this code hit on some its pages.

boesing commented 4 years ago

Thanks @xvilo!

Could you put a little bit more time into this and provide all necessary changes to achieve PHP 8.0 support? I've tried to add an easy checklist in #2 for this.

Our community would really benefit from your changes.

xvilo commented 4 years ago

@boesing thanks for your quick response, if have done the following as of now:

The last step in your mentioned issue is something I did not completely do, as I don't have time for this. At least one issue have been fixed initially.

xvilo commented 4 years ago

It seems Travis is not triggered anymore, any idea why?

boesing commented 4 years ago

@xvilo Yeah, laminas exceeded limits... We have to wait till the next "free period" starts. We are aware of this but actually didn't come up with a solution.

weierophinney commented 4 years ago

I've run tests against each of 7.3, 7.4, and 8.0 locally, using both latest and lowest dependencies. Tests all pass, as do CS-checks.

xvilo commented 4 years ago

Thanks for finishing it up @weierophinney