Review parameter names in ext/xmlreader #6255
Conversation
ext/xmlreader/php_xmlreader.stub.php
Outdated
|
|
||
| /** @return DOMNode|false|null */ | ||
| public function expand(?DOMNode $basenode = null) {} | ||
| public function expand(?DOMNode $document = null) {} |
There was a problem hiding this comment.
I'm not sure we can call this parameter $document (mainly because I'm not familar with ext/xmlreader)
There was a problem hiding this comment.
Off: If ext/dom is not available, XMLReader::expand() triggers a warning and returns null immediately, which whacks ZPP handling... That said, shouldn't we define the function itself conditionally instead?
There was a problem hiding this comment.
I'm not sure we can call this parameter
$document(mainly because I'm not familar with ext/xmlreader)
I wanted to call it $document because it is typically a DOMDocument that the created node will be associated with. It accepts more than just DOMDocument instances though, so maybe $baseNode is a better idea.
Off: If ext/dom is not available, XMLReader::expand() triggers a warning and returns null immediately, which whacks ZPP handling... That said, shouldn't we define the function itself conditionally instead?
Wow, I did not notice that before. What exactly do you mean with "whacks ZPP handling"? Are you referring to the parameters not being parsed and checked?
Having the function disappear completely also does not sound ideal. I think I would rather have it throwing an exception that clearly states what the problem is. This problem also makes me wonder why we have the possibility to compile PHP without DOM support in the first place. Dropping that possibility would be the simplest solution.
There was a problem hiding this comment.
I've addressed the zpp issue in faea5ab by making the function throw rather than warning and returning null.
This problem also makes me wonder why we have the possibility to compile PHP without DOM support in the first place. Dropping that possibility would be the simplest solution.
This is not possible, because dom requires libxml.
There was a problem hiding this comment.
... and libxml is not available on some of the supported platforms?
There was a problem hiding this comment.
... and libxml is not available on some of the supported platforms?
That's unlikely. However, a minimal PHP build should not have any dependencies beyond libc. Any dependencies a minimal PHP build needs have to be vendored, and we certainly wouldn't want to vendor libxml.
There was a problem hiding this comment.
Ah, ok. I am not familiar with minimal PHP builds. Anyways, thanks for fixing the ZPP issue!
ext/xmlreader/php_xmlreader.stub.php
Outdated
| @@ -71,11 +71,11 @@ public function setParserProperty(int $property, bool $value) {} | |||
| public function setRelaxNGSchema(?string $filename) {} | |||
|
|
|||
| /** @return bool */ | |||
| public function setRelaxNGSchemaSource(?string $source) {} | |||
| public function setRelaxNGSchemaSource(?string $value) {} | |||
There was a problem hiding this comment.
I think I'd keep $source here and in case of the next change. But let's wait for Nikita's preference too :)
There was a problem hiding this comment.
I agree that at least for the XML() function the name $value is not ideal. So yeah, keep $source, or possibly use $code.
There was a problem hiding this comment.
I just rebased on Nikita's ZPP fix and adjusted these parameter names.
dtakken
force-pushed
the
parameter_names_xmlreader
branch
from
f78de53 to
700612d
Compare
|
This looks good to me. Any more comments from @kocsismate? |
|
There is not any from my side! |
| @@ -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) {} | |||
There was a problem hiding this comment.
On the topic of $sqlite3, should the first parameter be called $XML? Can we maybe use $source in sqlite3 as well?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Next!