Skip to content

Review parameter names in ext/xmlreader #6255

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 1 commit into from
Closed

Review parameter names in ext/xmlreader #6255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 deletions ext/xmlreader/php_xmlreader.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public function getAttribute(string $name) {}
public function getAttributeNo(int $index) {}

/** @return string|null */
public function getAttributeNs(string $name, string $namespaceURI) {}
public function getAttributeNs(string $name, string $uri) {}

/** @return bool */
public function getParserProperty(int $property) {}
Expand All @@ -32,7 +32,7 @@ public function moveToAttribute(string $name) {}
public function moveToAttributeNo(int $index) {}

/** @return bool */
public function moveToAttributeNs(string $name, string $namespaceURI) {}
public function moveToAttributeNs(string $name, string $uri) {}

/** @return bool */
public function moveToElement() {}
Expand All @@ -47,10 +47,10 @@ public function moveToNextAttribute() {}
public function read() {}

/** @return bool */
public function next(?string $localname = null) {}
public function next(?string $name = null) {}

/** @return bool|XMLReader */
public static function open(string $URI, ?string $encoding = null, int $options = 0) {}
public static function open(string $uri, ?string $encoding = null, int $flags = 0) {}

/** @return string */
public function readInnerXml() {}
Expand All @@ -74,8 +74,8 @@ public function setRelaxNGSchema(?string $filename) {}
public function setRelaxNGSchemaSource(?string $source) {}

/** @return bool|XMLReader */
public static function XML(string $source, ?string $encoding = null, int $options = 0) {}
public static function XML(string $source, ?string $encoding = null, int $flags = 0) {}
Copy link
Member

Choose a reason for hiding this comment

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

On the topic of $sqlite3, should the first parameter be called $XML? Can we maybe use $source in sqlite3 as well?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the connection to sqlite3. $source here is in the sense of "source code" not in the sense of "source and target". Where would we be able to use $source in the sqlite API?

Copy link
Member

Choose a reason for hiding this comment

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

I was talking about the backup() method. The first parameter could be just $destination same as we have just $source here. I didn't mean the parameters are the same in these two extensions, just we should follow the same pattern.
The first parameter in xml() is actually an XML string, but we call it source, could we not call the first parameter in sqlite3::backup() just $destination to follow a similar pattern? Sorry for my confusing way of writing.

Copy link
Author

Choose a reason for hiding this comment

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

I'm a bit confused as well. Are you thinking of the name $source as "where it gets the data from"? That the $sqlite3 parameter in sqlite3::backup() and the $source parameter in xmlreader::xml() are related in that way?

Object instances like the $sqlite3 parameter are often named after the class that it is an instance of. Is that the source (I'm sorry) of your confusion?

Copy link
Member

Choose a reason for hiding this comment

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

No, I am not making a functional connection between these two extensions. I am purely talking about name choice. I.e. why have chosen $source instead of $xml?

Following the same logic, we can use the name $destination instead of $sqlite3. I know the type of the parameter is different, but I am only talking about naming convention.

Copy link
Author

Choose a reason for hiding this comment

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

why have chosen $source instead of $xml?

Because it is already clear from the context that it is expecting XML. That means we can use a name that is more generic and consistent with other extensions.

You do have a fair argument when you say that $sqlite3 should be renamed to something more generic, like $destination, for the same reason. It has been suggested to do so, but general consensus appears to be that the name should reflect the type of object in these cases.


/** @return DOMNode|false */
public function expand(?DOMNode $basenode = null) {}
public function expand(?DOMNode $baseNode = null) {}
}
14 changes: 7 additions & 7 deletions 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: 7d549429c217e5c3b9c36013b6737f4e8eeccbb6 */
* Stub hash: b5860285a7554c75780be7989bcbdeced6b557c8 */

ZEND_BEGIN_ARG_INFO_EX(arginfo_class_XMLReader_close, 0, 0, 0)
ZEND_END_ARG_INFO()
Expand All @@ -14,7 +14,7 @@ ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_class_XMLReader_getAttributeNs, 0, 0, 2)
ZEND_ARG_TYPE_INFO(0, name, IS_STRING, 0)
ZEND_ARG_TYPE_INFO(0, namespaceURI, IS_STRING, 0)
ZEND_ARG_TYPE_INFO(0, uri, IS_STRING, 0)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_class_XMLReader_getParserProperty, 0, 0, 1)
Expand Down Expand Up @@ -42,13 +42,13 @@ ZEND_END_ARG_INFO()
#define arginfo_class_XMLReader_read arginfo_class_XMLReader_close

ZEND_BEGIN_ARG_INFO_EX(arginfo_class_XMLReader_next, 0, 0, 0)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, localname, IS_STRING, 1, "null")
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, name, IS_STRING, 1, "null")
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_class_XMLReader_open, 0, 0, 1)
ZEND_ARG_TYPE_INFO(0, URI, IS_STRING, 0)
ZEND_ARG_TYPE_INFO(0, uri, IS_STRING, 0)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, encoding, IS_STRING, 1, "null")
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, options, IS_LONG, 0, "0")
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, flags, IS_LONG, 0, "0")
ZEND_END_ARG_INFO()

#define arginfo_class_XMLReader_readInnerXml arginfo_class_XMLReader_close
Expand All @@ -75,11 +75,11 @@ ZEND_END_ARG_INFO()
ZEND_BEGIN_ARG_INFO_EX(arginfo_class_XMLReader_XML, 0, 0, 1)
ZEND_ARG_TYPE_INFO(0, source, IS_STRING, 0)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, encoding, IS_STRING, 1, "null")
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, options, IS_LONG, 0, "0")
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, flags, IS_LONG, 0, "0")
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_class_XMLReader_expand, 0, 0, 0)
ZEND_ARG_OBJ_INFO_WITH_DEFAULT_VALUE(0, basenode, DOMNode, 1, "null")
ZEND_ARG_OBJ_INFO_WITH_DEFAULT_VALUE(0, baseNode, DOMNode, 1, "null")
ZEND_END_ARG_INFO()


Expand Down
2 changes: 1 addition & 1 deletion ext/xmlreader/tests/002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,6 @@ unlink($filename);

?>
--EXPECT--
XMLReader::open(): Argument #1 ($URI) cannot be empty
XMLReader::open(): Argument #1 ($uri) cannot be empty
books
books
2 changes: 1 addition & 1 deletion ext/xmlreader/tests/015-get-errors.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,5 @@ $reader->close();
unlink(__DIR__.'/015-get-errors.xml');
?>
--EXPECT--
XMLReader::getAttributeNs(): Argument #2 ($namespaceURI) cannot be empty
XMLReader::getAttributeNs(): Argument #2 ($uri) cannot be empty
ns1:num: 1
2 changes: 1 addition & 1 deletion ext/xmlreader/tests/015-move-errors.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,4 @@ $reader->close();
unlink(__DIR__.'/015-move-errors.xml');
?>
--EXPECT--
XMLReader::moveToAttributeNs(): Argument #2 ($namespaceURI) cannot be empty
XMLReader::moveToAttributeNs(): Argument #2 ($uri) cannot be empty