skip XML schema validation if libxml2 has been built without LIBXML_S…

[rcanavan]

libxml2 can be built without schema support (e.g. by configuring --without-schemas). While no PHP is currently available that would build with libxml/dom support with such a libxml2, we do have appropriate patches locally and intend to upstream them next week. Right now, we need to replace DOMDocument::schemaValidateSource with a stub function that always returns true so that PHPUnit does not fail with a validation error.

We would prefer if PHPUnit would skip validation if the schemaValidateSource() method is unavailable. This (untested) pull request should accomplish this.

[sebastianbergmann]

Could you please share the motivation behind the changes in php/php-src@master...rcanavan:php-src:libxml-minimal-build? Is the goal to reduce binary size, minimize attack surface, or something else?

I ask because, depending on the answer, it may affect whether PHPUnit needs to respond to this at all. PHPUnit is a development tool rather than something deployed in resource-constrained or hardened production environments, so it is not immediately clear to me that it should need to adapt its behaviour to accommodate minimal libxml2 builds. I would like to understand the use case better before deciding how to proceed.

However, I must say that I do not like the solution you propose in this pull request. Not only would it fail to perform the validation, it would also fail to inform the user.

I currently see three options:

1. Check whether schemaValidateSource() is available and error out if it is not (this would be my preference)
2. Check whether schemaValidateSource() is available, trigger a warning when it is not, and try to continue with loading an XML configuration file that was not validated
3. Do nothing and simply let PHPUnit crash when schemaValidateSource() is not available

[theseer]

I'm with option 2 in a way, actually. Or maybe a bit of 1.

The fact we validate the XML against a schema is a UX feature - and to avoid useless bug reports when people make typos in editing the configuration or using one that is simply to old and we cannot support anymore.

Silently skipping is a bad idea, and "crashing" feels wrong as well.

If we want to be strict, we could add a "--ignore-missing-schema-validation"-kind of switch and otherwise deny running. But the more UX friendly and probably less work kind of way is to create a warning that the validation was skipped and we do not want to get any bug reports in case something doesn't work the way the user expects. Pretty much like the "tainted" mode of the linux kernel. You do something we do not support, so do not bother us with issues ;)

But given this is the first time I ever hear of disabling/missing schema validation being an option, I do not believe the effort of adding more code than required to be justified. We also do not have to make it harder for the users though either. So, I guess I stick with option 2 as described.

[sebastianbergmann]

Just to be clear: I will not make any changes to adapt PHPUnit to the possibility that the schemaValidateSource() method may not be available until PHP can be built in such a way that the DOM extension is available but the schemaValidateSource() method is not.

From what I understand after chatting with @ndossche, PHP's configure currently errors when 1) the DOM extension is to be built, and 2) the libxml2 being linked against does not offer the API controlled by --without-schemas.

[rcanavan]

Thank you for taking the time to look at my pull request.

Could you please share the motivation behind the changes in php/php-src@master...rcanavan:php-src:libxml-minimal-build? Is the goal to reduce binary size, minimize attack surface, or something else?

The goal is to minimize the attack surface. We would also like to run our PHPUnit tests against the exact PHP, libxml2 etc. stack that ships in the finished product.

However, I must say that I do not like the solution you propose in this pull request. Not only would it fail to perform the validation, it would also fail to inform the user.

I currently see three options:

1. Check whether schemaValidateSource() is available and error out if it is not (this would be my preference)
2. Check whether schemaValidateSource() is available, trigger a warning when it is not, and try to continue with loading an XML configuration file that was not validated
   [...] add a "--ignore-missing-schema-validation"-kind of switch

I agree that a warning would be the bare minumum. There's another alternative that's also rubbish, namely replacing schemaValidateSource() in PHP with a stub that always returns true. That is admittedly what we're doing right now, since we're patching PHP anyway, but we can continue to use a plain PHPUnit.

[rcanavan]

Just to be clear: I will not make any changes to adapt PHPUnit to the possibility that the schemaValidateSource() method may not be available until PHP can be built in such a way that the DOM extension is available but the schemaValidateSource() method is not.

That's totally understandable, and we'll work on upstreaming patches to that effect to PHP soon.

From what I understand after chatting with @ndossche, PHP's configure currently errors when 1) the DOM extension is to be built, and 2) the libxml2 being linked against does not offer the API controlled by --without-schemas.

If my experiment with php master is relevant, that is not strictly true. configure has no checks for libxml schema support (or, for that matter, if --with-valid and --with-valid were included, but those cause the build to fail - more patches to upstream).