Skip to content

Promote warnings to exceptions in ext/xmlreader #6021

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Promote warnings to exceptions in ext/xmlreader #6021

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86c8b88
Promote warnings to exceptions in ext/xmlreader
kocsismate Aug 19, 2020
49fa70c
Fix review
kocsismate Aug 24, 2020
541fc02
Review round 2
kocsismate Aug 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 42 additions & 36 deletions ext/xmlreader/php_xmlreader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ zval *xmlreader_write_property(zend_object *object, zend_string *name, zval *val
hnd = zend_hash_find_ptr(obj->prop_handler, name);
}
if (hnd != NULL) {
php_error_docref(NULL, E_WARNING, "Cannot write to read-only property");
zend_throw_error(NULL, "Cannot write to read-only property");
} else {
value = zend_std_write_property(object, name, value, cache_slot);
}
Expand Down Expand Up @@ -391,8 +391,8 @@ static void php_xmlreader_string_arg(INTERNAL_FUNCTION_PARAMETERS, xmlreader_rea
}

if (!name_len) {
php_error_docref(NULL, E_WARNING, "Argument cannot be an empty string");
RETURN_FALSE;
zend_argument_value_error(1, "cannot be empty");
RETURN_THROWS();
}

id = ZEND_THIS;
Expand Down Expand Up @@ -480,8 +480,8 @@ static void php_xmlreader_set_relaxng_schema(INTERNAL_FUNCTION_PARAMETERS, int t
}

if (source != NULL && !source_len) {
php_error_docref(NULL, E_WARNING, "Schema data source is required");
RETURN_FALSE;
zend_argument_value_error(1, "cannot be empty");
RETURN_THROWS();
}

id = ZEND_THIS;
Expand Down Expand Up @@ -509,9 +509,8 @@ static void php_xmlreader_set_relaxng_schema(INTERNAL_FUNCTION_PARAMETERS, int t
}
}

php_error_docref(NULL, E_WARNING, "Unable to set schema. This must be set prior to reading or schema contains errors.");

RETURN_FALSE;
zend_throw_error(NULL, "Unable to set schema. This must be set prior to reading or schema contains errors.");
Copy link
Member

Choose a reason for hiding this comment

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

"schema contains errors" doesn't sound like a promotable condition.

Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be split into the error condition for !intern || !intern->ptr (can throw) and the one for retval != 0 (cannot throw).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the recommendation, I managed to separate them, and I added test coverage for both the warning and the exception.

RETURN_THROWS();
#else
php_error_docref(NULL, E_WARNING, "No Schema support built into libxml.");

Expand Down Expand Up @@ -585,9 +584,14 @@ PHP_METHOD(XMLReader, getAttributeNs)
RETURN_THROWS();
}

if (name_len == 0 || ns_uri_len == 0) {
php_error_docref(NULL, E_WARNING, "Attribute Name and Namespace URI cannot be empty");
RETURN_FALSE;
if (name_len == 0) {
zend_argument_value_error(1, "cannot be empty");
RETURN_THROWS();
}

if (ns_uri_len == 0) {
zend_argument_value_error(2, "cannot be empty");
RETURN_THROWS();
}

id = ZEND_THIS;
Expand Down Expand Up @@ -622,8 +626,8 @@ PHP_METHOD(XMLReader, getParserProperty)
retval = xmlTextReaderGetParserProp(intern->ptr,property);
}
if (retval == -1) {
php_error_docref(NULL, E_WARNING, "Invalid parser property");
RETURN_FALSE;
zend_argument_value_error(1, "must be a valid parser property");
RETURN_THROWS();
}

RETURN_BOOL(retval);
Expand Down Expand Up @@ -660,8 +664,8 @@ PHP_METHOD(XMLReader, moveToAttribute)
}

if (name_len == 0) {
php_error_docref(NULL, E_WARNING, "Attribute Name is required");
RETURN_FALSE;
zend_argument_value_error(1, "cannot be empty");
RETURN_THROWS();
}

id = ZEND_THIS;
Expand Down Expand Up @@ -719,9 +723,14 @@ PHP_METHOD(XMLReader, moveToAttributeNs)
RETURN_THROWS();
}

if (name_len == 0 || ns_uri_len == 0) {
php_error_docref(NULL, E_WARNING, "Attribute Name and Namespace URI cannot be empty");
RETURN_FALSE;
if (name_len == 0) {
zend_argument_value_error(1, "cannot be empty");
RETURN_THROWS();
}

if (ns_uri_len == 0) {
zend_argument_value_error(2, "cannot be empty");
RETURN_THROWS();
}

id = ZEND_THIS;
Expand Down Expand Up @@ -781,8 +790,7 @@ PHP_METHOD(XMLReader, read)
}
}

php_error_docref(NULL, E_WARNING, "Load Data before trying to read");
RETURN_FALSE;
zend_throw_error(NULL, "Data must be loaded before reading");
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: Rewrite this as early return?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense for me!

}
/* }}} */

Expand Down Expand Up @@ -816,8 +824,7 @@ PHP_METHOD(XMLReader, next)
}
}

php_error_docref(NULL, E_WARNING, "Load Data before trying to read");
RETURN_FALSE;
zend_throw_error(NULL, "Data must be loaded before reading");
}
/* }}} */

Expand Down Expand Up @@ -848,8 +855,8 @@ PHP_METHOD(XMLReader, open)
}

if (!source_len) {
php_error_docref(NULL, E_WARNING, "Empty string supplied as input");
RETURN_FALSE;
zend_argument_value_error(1, "cannot be empty");
RETURN_THROWS();
}

valid_file = _xmlreader_get_valid_file_path(source, resolved_path, MAXPATHLEN );
Expand Down Expand Up @@ -920,8 +927,8 @@ PHP_METHOD(XMLReader, setSchema)
}

if (source != NULL && !source_len) {
php_error_docref(NULL, E_WARNING, "Schema data source is required");
RETURN_FALSE;
zend_argument_value_error(1, "cannot be empty");
RETURN_THROWS();
}

id = ZEND_THIS;
Expand All @@ -935,9 +942,8 @@ PHP_METHOD(XMLReader, setSchema)
}
}

php_error_docref(NULL, E_WARNING, "Unable to set schema. This must be set prior to reading or schema contains errors.");

RETURN_FALSE;
zend_throw_error(NULL, "Unable to set schema. This must be set prior to reading or schema contains errors.");
RETURN_THROWS();
#else
php_error_docref(NULL, E_WARNING, "No Schema support built into libxml.");

Expand Down Expand Up @@ -967,8 +973,8 @@ PHP_METHOD(XMLReader, setParserProperty)
retval = xmlTextReaderSetParserProp(intern->ptr,property, value);
}
if (retval == -1) {
php_error_docref(NULL, E_WARNING, "Invalid parser property");
RETURN_FALSE;
zend_argument_value_error(1, "must be a valid parser property");
RETURN_THROWS();
}

RETURN_TRUE;
Expand Down Expand Up @@ -1022,8 +1028,8 @@ PHP_METHOD(XMLReader, XML)
}

if (!source_len) {
php_error_docref(NULL, E_WARNING, "Empty string supplied as input");
RETURN_FALSE;
zend_argument_value_error(1, "cannot be empty");
RETURN_THROWS();
}

inputbfr = xmlParserInputBufferCreateMem(source, source_len, XML_CHAR_ENCODING_NONE);
Expand Down Expand Up @@ -1105,7 +1111,7 @@ PHP_METHOD(XMLReader, expand)
node = xmlTextReaderExpand(intern->ptr);

if (node == NULL) {
php_error_docref(NULL, E_WARNING, "An Error Occurred while expanding ");
php_error_docref(NULL, E_WARNING, "An Error Occurred while expanding");
RETURN_FALSE;
} else {
nodec = xmlDocCopyNode(node, docp, 1);
Expand All @@ -1117,8 +1123,8 @@ PHP_METHOD(XMLReader, expand)
}
}
} else {
php_error_docref(NULL, E_WARNING, "Load Data before trying to expand");
RETURN_FALSE;
zend_throw_error(NULL, "Data must be loaded before expanding");
RETURN_THROWS();
}
#else
php_error(E_WARNING, "DOM support is not enabled");
Expand Down
4 changes: 2 additions & 2 deletions ext/xmlreader/php_xmlreader.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class XMLReader
/** @return bool */
public function close() {}

/** @return string|null|false */
/** @return string|null */
public function getAttribute(string $name) {}

/** @return string|null */
Expand All @@ -22,7 +22,7 @@ public function getParserProperty(int $property) {}
/** @return bool */
public function isValid() {}

/** @return string|null|false */
/** @return string|null */
public function lookupNamespace(string $prefix) {}

/** @return bool */
Expand Down
2 changes: 1 addition & 1 deletion ext/xmlreader/php_xmlreader_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* This is a generated file, edit the .stub.php file instead.
* Stub hash: 8bdec18c4ad8574fb1d3e4baca928949d5ec2438 */
* Stub hash: 90e6d525ba87399c54f36965ebf18dbf65084617 */

ZEND_BEGIN_ARG_INFO_EX(arginfo_class_XMLReader_close, 0, 0, 0)
ZEND_END_ARG_INFO()
Expand Down
13 changes: 9 additions & 4 deletions ext/xmlreader/tests/001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@ while ($reader->read()) {
}
$xmlstring = '';
$reader = new XMLReader();
$reader->XML($xmlstring);

try {
$reader->XML($xmlstring);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

?>
--EXPECTF--
--EXPECT--
books
books

Warning: XMLReader::XML(): Empty string supplied as input in %s on line %d
XMLReader::XML(): Argument #1 ($source) cannot be empty
10 changes: 7 additions & 3 deletions ext/xmlreader/tests/002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ $xmlstring = '<?xml version="1.0" encoding="UTF-8"?>
file_put_contents($filename, $xmlstring);

$reader = new XMLReader();
if ($reader->open('')) exit();
try {
$reader->open('');
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

$reader = new XMLReader();
if (!$reader->open($filename)) {
Expand All @@ -31,7 +35,7 @@ $reader->close();
unlink($filename);

?>
--EXPECTF--
Warning: XMLReader::open(): Empty string supplied as input in %s on line %d
--EXPECT--
XMLReader::open(): Argument #1 ($URI) cannot be empty
books
books
14 changes: 8 additions & 6 deletions ext/xmlreader/tests/003-get-errors.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,12 @@ while ($reader->read()) {
echo $reader->value . "\n";

// Test for call with an empty string argument
$attr = $reader->getAttribute('');
var_dump($attr);
try {
$reader->getAttribute('');
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

// Ensure that node pointer has not changed position
echo $reader->name . ": ";
echo $reader->value . "\n";
Expand Down Expand Up @@ -61,13 +65,11 @@ $reader->close();
<?php
unlink(__DIR__.'/003-get-errors.xml');
?>
--EXPECTF--
--EXPECT--
book
bool(true)
num: 1

Warning: XMLReader::getAttribute(): Argument cannot be an empty string in %s on line %d
bool(false)
XMLReader::getAttribute(): Argument #1 ($name) cannot be empty
num: 1
NULL
num: 1
Expand Down
14 changes: 8 additions & 6 deletions ext/xmlreader/tests/003-move-errors.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,12 @@ while ($reader->read()) {
echo $reader->value . "\n";

// Test for call with an empty string argument
$attr = $reader->moveToAttribute('');
var_dump($attr);
try {
$reader->moveToAttribute('');
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

// Ensure that node pointer has not changed position
echo $reader->name . ": ";
echo $reader->value . "\n";
Expand Down Expand Up @@ -60,13 +64,11 @@ $reader->close();
<?php
unlink(__DIR__.'/003-move-errors.xml');
?>
--EXPECTF--
--EXPECT--
book
bool(true)
num: 1

Warning: XMLReader::moveToAttribute(): Attribute Name is required in %s on line %d
bool(false)
XMLReader::moveToAttribute(): Argument #1 ($name) cannot be empty
num: 1
bool(false)
num: 1
Expand Down
12 changes: 7 additions & 5 deletions ext/xmlreader/tests/003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,18 @@ while ($reader->read()) {

var_dump($reader->moveToAttributeNo(20));
var_dump($reader->moveToAttribute('missing-attribute'));
var_dump($reader->moveToAttribute(''));
try {
$reader->moveToAttribute('');
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}
}
}
}
$reader->close();
unlink($filename);
?>
--EXPECTF--
--EXPECT--
num: 1
idx: 2
num: 1
Expand All @@ -84,6 +88,4 @@ num: 1
idx: 2
bool(false)
bool(false)

Warning: XMLReader::moveToAttribute(): Attribute Name is required in %s on line %d
bool(false)
XMLReader::moveToAttribute(): Argument #1 ($name) cannot be empty
12 changes: 7 additions & 5 deletions ext/xmlreader/tests/007.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,15 @@ $reader->close();
$reader = new XMLReader();
$reader->XML($xmlstring);

if ($reader->setRelaxNGSchema('')) {
echo 'failed';
try {
$reader->setRelaxNGSchema('');
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

$reader->close();
?>
--EXPECTF--
--EXPECT--
file relaxNG: ok
string relaxNG: ok

Warning: XMLReader::setRelaxNGSchema(): Schema data source is required in %s on line %d
XMLReader::setRelaxNGSchema(): Argument #1 ($filename) cannot be empty
Loading