[Zegnat]

Closes #285, fixes #268.

Inspired on the work I previously did for a different library. It looks like all the tests pass.

I also updated php56.Dockerfile to be able to actually install and run with PHP 5.6. When I tried to run that one as it was to confirm backwards compatibility it failed because some of the underlying infrastructure had gotten extremely outdated (such as the Debian mirrors it depended on).

As a final addition this also enabled PHPStan static analysis of the code, which would have caught some of the review comments we got here. It also helped to clean up some existing PHP DocBlocks that otherwise might have been misinforming users.

[Zegnat]

@jtojnar good catches! I had the (string) cast on all the attribute accessors, but not on localName and textContent. So those are in now as per your suggestions.

I have also added PHPStan here, so we can actually run the static analysis that you hinted at. I do not configure that often, so I went with the most basic setup possible. If you have suggestions there I appreciate them. With this all the DocBlock definitions are hopefully correct.

[jtojnar]

Might be cleaner to use @param-out #274

[Zegnat]

First time hearing about @param-out. It does not seem to exist in the PHPDoc reference that I am used to. So I guess it was introduced by PHPStan?

It looks like Psalm introduced it too to be used when “a by-ref type is different from the one that entered”. Which has had people point out that you need to include both @param and @param-out.

For reference, the format I am using here with & on the variable name was taken from WikiMedia’s code style: code search on wikimedia/mediawiki for @param [^ ]+ &$.

I have no strong feelings. I would also be more than happy to have this merged separately and then get #274 merged, maybe with a more complete review of all the PHPDocs in the library.

[jtojnar]

Yeah, it is PHPStan and psalm specific.

The difference is that in most of the mediawiki examples, the argument is mandatory, whereas here, the input param is either (default) null or a (unitialized) array variable (not that we care since the reference will be overwritten with the out type) but the output is always array (I am not even sure if the curl_getinfo call can actually fail here).

There are some examples in mediawiki but it does not look like they bother with encoding that the output will be non-null:
https://github.com/search?q=repo%3Awikimedia%2Fmediawiki+%2F%26%5C%24%5Ba-z%5D%2B+%3D%2F&type=code

[Zegnat]

I am not even sure if the curl_getinfo call can actually fail here

Not sure if anyone knows. Would have to inspect the bindings / the curl behaviour 😆 It should not as we call it after a false check on the curl_exec results. But not 100% certain. The safe bet, to match the PHP documentation, is to always encode false as a possible result value unless we ourselves set a different value for failure.

One thing we can do is match the original &array doc rather than trying to fix this in some other way:

/**
 * …
 * @param array &$curlInfo (optional) the results of curl_getinfo will be placed in this variable for debugging
 * …
 */
function fetch($url, $convertClassic = true, &$curlInfo = array()) {
    $curlInfo = array();
    // …
    $info = $curlInfo = (curl_getinfo($ch) ?: array());
    // …
}

Explanation:

1. We have to have a default value to make the argument optional, for PHP’s sake null was fine but PHPStan requires an array or we have to change the doc.
2. At the top of the function we reset the variable to an empty array. This ensures that reading the variable afterwards always gives an array, even if the curl request failed.
3. When we assign the variable later, we fall back to an empty array if curl_getinfo happens to fail. (I vaguely think this is default for PHP 8 anyway, where it never returns false.)

At that point the in and out values do not matter much any more. Either you pass no variable, or whatever you pass is assigned an array.

[Zegnat]

The above is now what I implemented in 3c42af1, which I am personally quiet happy with. If we want to do a more complete PR of just PHPStan and DocBlocks, I would prefer that over rolling even more into this one.

[jtojnar]

Perhaps move this into a separate PR so that we can merge it into 0.5.1, which @gRegorLove wants to do before BC breaks in 0.6.