lat9 / upsxml

UPS-XML for Zen Cart
3 stars 4 forks source link

class needs to be renamed for use with php 8.0 #27

Closed proseLA closed 2 years ago

proseLA commented 2 years ago

that took a little time to figure out....

php 8 introduced a new fully opaque class as seen here:

https://www.php.net/manual/en/class.xmlparser.php

in order for this plugin to run as is, one would need to rename this class and its usage:

https://github.com/lat9/upsxml/blob/5a0dfad3f46039a222045bb73f8a2b52d83f0889/includes/classes/xmldocument.php#L240

https://github.com/lat9/upsxml/blob/5a0dfad3f46039a222045bb73f8a2b52d83f0889/includes/modules/shipping/upsxml.php#L910

https://github.com/lat9/upsxml/blob/5a0dfad3f46039a222045bb73f8a2b52d83f0889/includes/modules/shipping/upsxml.php#L1039

the alternative would be a refactor making use of the new XML parse functions with php 8.0

lat9 commented 2 years ago

FWIW, I'll take the 'fast-path' and simply rename all the classes to start with ups so that there's (hopefully) no further collision with the PHP base. Thanks for the report, BTW, I can imagine the amount of head-scratching involved in finding the root-cause.

lat9 commented 2 years ago

Noting that if (???) there are other plugins that also provide /includes/classes/xmldocument.php there will be a clash, but they would have the same issue on PHP 8.0+ anyway.

proseLA commented 2 years ago

FWIW, I'll take the 'fast-path' and simply rename all the classes to start with ups so that there's (hopefully) no further collision with the PHP base. Thanks for the report, BTW, I can imagine the amount of head-scratching involved in finding the root-cause.

that's what i would do as well.

Noting that if (???) there are other plugins that also provide /includes/classes/xmldocument.php there will be a clash, but they would have the same issue on PHP 8.0+ anyway.

a brief search would indicate this was something carried over from the osCommerce days.

i have done plenty of XML work over the years and have never used these classes, so i think we are safe. but as you say, the problem would still exist in any 8.0+ install...