Skip to content

Promote warnings to exceptions in ext/simplexml #6011

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.

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 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 7 additions & 1 deletion Zend/zend_object_handlers.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,13 @@ typedef zval *(*zend_object_write_property_t)(zend_object *object, zend_string *
typedef void (*zend_object_write_dimension_t)(zend_object *object, zval *offset, zval *value);


/* Used to create pointer to the property of the object, for future direct r/w access */
/* Used to create pointer to the property of the object, for future direct r/w access.
* May return one of:
* * A zval pointer, without incrementing the reference count.
* * &EG(error_zval), if an exception has been thrown.
* * NULL, if acquiring a direct pointer is not possible.
* In this case, the VM will fall back to using read_property and write_property.
*/
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 this explanation, it's really useful! Of course, I'm grateful for the fix, too :)

typedef zval *(*zend_object_get_property_ptr_ptr_t)(zend_object *object, zend_string *member, int type, void **cache_slot);

/* Used to check if a property of the object exists */
Expand Down
38 changes: 19 additions & 19 deletions ext/simplexml/simplexml.c
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
* and could also be E_PARSE, but we use this only during parsing
* and this is during runtime.
*/
zend_throw_error(NULL, "Cannot create unnamed attribute");
zend_throw_error(NULL, "Cannot append to an attribute list");
Copy link
Member

Choose a reason for hiding this comment

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

I'd leave only the first line of the preceding comment.

return &EG(error_zval);
}
goto long_dim;
Expand All @@ -436,7 +436,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
}

if (!Z_STRLEN_P(member)) {
php_error_docref(NULL, E_WARNING, "Cannot write or create unnamed %s", attribs ? "attribute" : "element");
zend_value_error("Cannot create %s with an empty name", attribs ? "attribute" : "element");
if (member == &tmp_zv) {
zval_ptr_dtor_str(&tmp_zv);
}
Expand Down Expand Up @@ -464,7 +464,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
* and could also be E_PARSE, but we use this only during parsing
* and this is during runtime.
*/
zend_throw_error(NULL, "Cannot create unnamed attribute");
zend_value_error("Cannot append to an attribute list");
return &EG(error_zval);
}
if (attribs && !node && sxe->iter.type == SXE_ITER_ELEMENT) {
Expand All @@ -489,8 +489,8 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
if (Z_OBJCE_P(value) == sxe_class_entry) {
zval zval_copy;
if (sxe_object_cast_ex(Z_OBJ_P(value), &zval_copy, IS_STRING) == FAILURE) {
zend_error(E_ERROR, "Unable to cast node to string");
/* FIXME: Should not be fatal */
zend_throw_error(NULL, "Unable to cast node to string");
return &EG(error_zval);
}

value_str = Z_STR(zval_copy);
Expand All @@ -501,7 +501,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
if (member == &tmp_zv) {
zval_ptr_dtor_str(&tmp_zv);
}
zend_error(E_WARNING, "It is not yet possible to assign complex types to %s", attribs ? "attributes" : "properties");
zend_type_error("It's not possible to assign a complex type to %s, %s given", attribs ? "attributes" : "properties", zend_zval_type_name(value));
return &EG(error_zval);
}
}
Expand Down Expand Up @@ -656,7 +656,7 @@ static zval *sxe_property_get_adr(zend_object *object, zend_string *zname, int f
}
ZVAL_STR(&member, zname);
if (sxe_prop_dim_write(object, &member, NULL, 1, 0, &node) == &EG(error_zval)) {
return NULL;
return &EG(error_zval);
}
type = SXE_ITER_NONE;
name = NULL;
Expand Down Expand Up @@ -1684,8 +1684,8 @@ SXE_METHOD(addChild)
}

if (qname_len == 0) {
php_error_docref(NULL, E_WARNING, "Element name is required");
return;
zend_argument_value_error(1, "cannot be empty");
RETURN_THROWS();
}

sxe = Z_SXEOBJ_P(ZEND_THIS);
Expand Down Expand Up @@ -1749,8 +1749,8 @@ SXE_METHOD(addAttribute)
}

if (qname_len == 0) {
php_error_docref(NULL, E_WARNING, "Attribute name is required");
return;
zend_argument_value_error(1, "cannot be empty");
RETURN_THROWS();
}

sxe = Z_SXEOBJ_P(ZEND_THIS);
Expand Down Expand Up @@ -2274,8 +2274,8 @@ PHP_FUNCTION(simplexml_load_file)
}

if (ZEND_LONG_EXCEEDS_INT(options)) {
php_error_docref(NULL, E_WARNING, "Invalid options");
RETURN_FALSE;
zend_argument_value_error(3, "is too large");
RETURN_THROWS();
}

docp = xmlReadFile(filename, NULL, (int)options);
Expand Down Expand Up @@ -2319,16 +2319,16 @@ PHP_FUNCTION(simplexml_load_string)
}

if (ZEND_SIZE_T_INT_OVFL(data_len)) {
php_error_docref(NULL, E_WARNING, "Data is too long");
RETURN_FALSE;
zend_argument_value_error(1, "is too long");
RETURN_THROWS();
}
if (ZEND_SIZE_T_INT_OVFL(ns_len)) {
php_error_docref(NULL, E_WARNING, "Namespace is too long");
RETURN_FALSE;
zend_argument_value_error(4, "is too long");
RETURN_THROWS();
}
if (ZEND_LONG_EXCEEDS_INT(options)) {
php_error_docref(NULL, E_WARNING, "Invalid options");
RETURN_FALSE;
zend_argument_value_error(3, "is too large");
RETURN_THROWS();
}

docp = xmlReadMemory(data, (int)data_len, NULL, NULL, (int)options);
Expand Down
4 changes: 2 additions & 2 deletions ext/simplexml/simplexml.stub.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public function rewind() {}
/** @return bool */
public function valid() {}

/** @return ?SimpleXMLElement */
/** @return SimpleXMLElement|null */
public function current() {}

/** @return string|false */
Expand All @@ -71,7 +71,7 @@ public function next() {}
/** @return bool */
public function hasChildren() {}

/** @return ?SimpleXMLElement */
/** @return SimpleXMLElement|null */
public function getChildren() {}
}

Expand Down
2 changes: 1 addition & 1 deletion ext/simplexml/simplexml_arginfo.h
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: fbe25d8a7a0a1de0cbd5dc9118e77a2e8d5dbd67 */
* Stub hash: 953089f230247acf18b9ac48c0a4c516d144a987 */

ZEND_BEGIN_ARG_WITH_RETURN_OBJ_TYPE_MASK_EX(arginfo_simplexml_load_file, 0, 1, SimpleXMLElement, MAY_BE_FALSE)
ZEND_ARG_TYPE_INFO(0, filename, IS_STRING, 0)
Expand Down
22 changes: 13 additions & 9 deletions ext/simplexml/tests/012.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@ EOF;

$sxe = simplexml_load_string($xml);

try {
$sxe[""] = "value";
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

$sxe[""] = "warning";
$sxe["attr"] = "value";

echo $sxe->asXML();
Expand All @@ -24,19 +28,19 @@ $sxe["attr"] = "new value";

echo $sxe->asXML();

$sxe[] = "error";
try {
$sxe[] = "error";
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

__HALT_COMPILER();
?>
===DONE===
--EXPECTF--
Warning: main(): Cannot write or create unnamed attribute in %s012.php on line %d
--EXPECT--
Cannot create attribute with an empty name
<?xml version="1.0" encoding="ISO-8859-1"?>
<foo attr="value"/>
<?xml version="1.0" encoding="ISO-8859-1"?>
<foo attr="new value"/>

Fatal error: Uncaught Error: Cannot create unnamed attribute in %s012.php:%d
Stack trace:
#0 {main}
thrown in %s012.php on line %d
Cannot append to an attribute list
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,16 @@ Havard Eide <[email protected]>
--FILE--
<?php
$a = new SimpleXMLElement("<php>testfest</php>");
$a->addAttribute( "", "" );

try {
$a->addAttribute( "", "" );
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

echo $a->asXML();
?>
--EXPECTF--
Warning: SimpleXMLElement::addAttribute(): Attribute name is required in %s on line %d
--EXPECT--
SimpleXMLElement::addAttribute(): Argument #1 ($qualifiedName) cannot be empty
<?xml version="1.0"?>
<php>testfest</php>
13 changes: 8 additions & 5 deletions ext/simplexml/tests/SimpleXMLElement_xpath_4.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@ if (PHP_INT_SIZE != 8) die("skip this test is for 64bit platforms only");
?>
--FILE--
<?php
$xml = simplexml_load_string("XXXXXXX^",$x,0x6000000000000001);
var_dump($xml);

try {
simplexml_load_string("XXXXXXX^", $x, 0x6000000000000001);
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

?>
--EXPECTF--
Warning: Undefined variable $x in %s on line %d

Warning: simplexml_load_string(): Invalid options in %s on line %d
bool(false)
simplexml_load_string(): Argument #3 ($options) is too large
15 changes: 10 additions & 5 deletions ext/simplexml/tests/bug37076_1.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,18 @@ Bug #37076 (SimpleXML ignores .=) (appending to unnamed attribute)
<?php if (!extension_loaded("simplexml")) print "skip"; ?>
--FILE--
<?php

$xml = simplexml_load_string("<root><foo /></root>");
$xml->{""} .= "bar";

try {
$xml->{""} .= "bar";
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

print $xml->asXML();
?>
--EXPECTF--
Warning: main(): Cannot write or create unnamed element in %s on line %d

Warning: main(): Cannot write or create unnamed element in %s on line %d
--EXPECT--
Cannot create element with an empty name
<?xml version="1.0"?>
<root><foo/></root>
10 changes: 7 additions & 3 deletions ext/simplexml/tests/bug38406.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ $item->otherAttribute = $item->attribute;
var_dump($item->otherAttribute);

$a = array();
$item->$a = new stdclass;

try {
$item->$a = new stdclass;
} catch (TypeError $exception) {
echo $exception->getMessage() . "\n";
}

echo "Done\n";
?>
Expand All @@ -28,6 +33,5 @@ object(SimpleXMLElement)#%d (1) {
}

Warning: Array to string conversion in %s on line %d

Warning: It is not yet possible to assign complex types to properties in %s on line %d
It's not possible to assign a complex type to properties, stdClass given
Done