Declare tentative return types for ext/dom

[kocsismate]

No description provided.

[nikic]

Something that bothers me about the DOM case is that this puts a number of false return values into the contract that are only relevant without strict errors. Such as the one here. Once the return types are in place, dropping |false will be breaking change :/

[nikic]

It looks like strictErrorChecking has actually been removed from the DOM spec, so I have some hope that we can simply drop support for it. Needs some investigation to understand what the impact is and maybe looking at some of the current uses (https://beta.grep.app/search?q=-%3EstrictErrorChecking%5Cs%2A%3D%5Cs%2Afalse&regexp=true).

[nikic]

Well, at least the curl part of this looks good to me :)

[TysonAndre]

Not familiar with this functionality, but noticed randomly: I think never would be more appropriate, but because never is added in 8.1, it would (somewhat) prevent overriding this in everything older than 8.1

Then again, https://developer.mozilla.org/en-US/docs/Web/API/DOMImplementation mentions that no browser ever implemented getFeature and it was removed from the spec, so this actually makes sense, but documenting the reason for a getter returning void (in c file and .stub.php file) would avoid confusion.

https://www.php.net/domimplementation mentions the method is undocumented

Maybe keep it as : void and add @return never

Aside: Not implemented may make more sense than"Not yet implemented" but this is way out of scope.

PHP_METHOD(DOMImplementation, getFeature)
{
	size_t feature_len, version_len;
	char *feature, *version;

	if (zend_parse_parameters(ZEND_NUM_ARGS(), "ss", &feature, &feature_len, &version, &version_len) == FAILURE) {
		RETURN_THROWS();
	}

	DOM_NOT_IMPLEMENTED(); // macro for zend_throw_error(NULL, "Not yet implemented");
}

[kocsismate]

@beberlei Do you think this function will be ever implemented? If not, and we don't want child classes to implement it, then never would be a good choice, otherwise I'm fine with mixed.

[kocsismate]

I've just gone with never

[kocsismate]

Well, at least the curl part of this looks good to me :)

I exploded the changes into 3 separate commits, so that I can iterate on ext/dom, but apply the rest directly.

[nikic]

I exploded the changes into 3 separate commits, so that I can iterate on ext/dom, but apply the rest directly.

Right, the ext/curl and ext/fileinfo parts look good to me.

[nikic]

I've been looking into this a bit, and can only say that error handling in dom is quite broken. Even with strictErrorChecking dropped there's a bunch of RETURN_FALSE that either don't match the DOM spec, or are for libxml internal errors.

For example, DOMDocument::createDocumentFragment() is spec'd to have no error conditions, but of course memory allocation in libxml can fail -- what should we be doing in that case? The current "return false" seems like a phenomenally bad way to handle it, in that people can only encounter this return value in very unusual situations that cannot be tested for. My current thinking is that we should be throwing a fatal error in this case, the same as we would do for other out-of-memory conditions in PHP.

[kocsismate]

I've been looking into this a bit, and can only say that error handling in dom is quite broken. Even with strictErrorChecking dropped there's a bunch of RETURN_FALSE that either don't match the DOM spec, or are for libxml internal errors.

Would you be ok if we didn't add a tentative return type for methods which rely on strictErrorChecking, so that we could get rid of more false returns in the future more easily?

[nikic]

@kocsismate I'd be okay with adding tentative types for all methods where we currently match the DOM spec.

[kocsismate]

I'd be okay with adding tentative types for all methods where we currently match the DOM spec.

Hopefully it's done now.