File: add new File::findExtendedInterfaceNames() utility method
#2128
Conversation
There was a problem hiding this comment.
Thank you for your effort. I took a small look at the changes and wonder, if we need a new method or if findImplementedInterfaceNames could be used with an interface too. I assume you chose a new method because of the clearer naming and for extending the File class with a new public method instead of changing existing method logic. I think I prefer that too even though File gets bigger by it.
Any thoughts by other reviewers?
src/Files/File.php
Outdated
| if ($name === '') { | ||
| $interfaces = $this->examineObjectDeclarationSignature($stackPtr, $validStructures, T_EXTENDS); | ||
|
|
||
| if (empty($interfaces) === true) { |
There was a problem hiding this comment.
I understand this is for consistency, but would returning an empty array be better than returning false in the if-block?
There was a problem hiding this comment.
From the end-user (= sniff developers) point of view, the difference is negligible, if (empty($interfacesNames)) will catch both false as well as en empty array.
What's better is a matter of opinion and in this case, I've elected consistency with the similar methods over always having the same type of return.
It can be adjusted easily enough and I'd be happy to do so if @gsherwood would deem always returning an array appropriate.
…multiple interfaces Interfaces can extend multiple other interfaces. Looking at the sniff code, no changes are needed in the sniff to account for this, however, adding an additional unit test to warrant against sniff changes in the future which may not take this into account. N.B.: I've verified all other usages of the `T_INTERFACE` token in the whole of WPCS and this was the only one which looked at `extends/implements`, so the only one for which interfaces extending multiple other interfaces could be problematic. See: * http://php.net/manual/en/language.oop5.interfaces.php#example-208 Loosely related to: * squizlabs/PHP_CodeSniffer#2128
In my opinion,
The new So as both the variables are different, I don't think it makes sense to try and get this into one method. |
jrfnl
force-pushed
the
feature/fix-findextendedclass-method
branch
from
946fab1
to
45db0c4
Compare
As I've just looked at the `findExtendedClassName()` method again for PR 2127, I remembered that SenseException reported another issue with it a while back via Twitter: https://twitter.com/SenseException/status/997988691468484609. Summary: interfaces can extend more than one interface (in contrast to classes) and the `findExtendedClassName()` method does not allow for that. See: http://php.net/manual/en/language.oop5.interfaces.php#example-208 To allow for this from within the existing `findExtendedClassName()` method would change the function signature from `@return string|false` to `@return string|array|false`, thus breaking the API. So, instead, I've elected to add a new `File::findExtendedInterfaceNames()` method and to recommend using that method for interfaces in the documentation of the `findExtendedClassName()` method . As the logic needed for these two methods, as well as for the `findImplementedInterfaces()` method, is basically the same, I've extracted the logic out to a private `File::examineObjectDeclarationSignature()` method which is now called by all three functions to DRY out the code. Includes dedicated unit tests for the new method.
jrfnl
force-pushed
the
feature/fix-findextendedclass-method
branch
from
45db0c4
to
dcc8223
Compare
…multiple interfaces Interfaces can extend multiple other interfaces. Looking at the sniff code, no changes are needed in the sniff to account for this, however, adding an additional unit test to warrant against sniff changes in the future which may not take this into account. N.B.: I've verified all other usages of the `T_INTERFACE` token in the whole of WPCS and this was the only one which looked at `extends/implements`, so the only one for which interfaces extending multiple other interfaces could be problematic. See: * http://php.net/manual/en/language.oop5.interfaces.php#example-208 Loosely related to: * squizlabs/PHP_CodeSniffer#2128
…multiple interfaces Interfaces can extend multiple other interfaces. Looking at the sniff code, no changes are needed in the sniff to account for this, however, adding an additional unit test to warrant against sniff changes in the future which may not take this into account. N.B.: I've verified all other usages of the `T_INTERFACE` token in the whole of WPCS and this was the only one which looked at `extends/implements`, so the only one for which interfaces extending multiple other interfaces could be problematic. See: * http://php.net/manual/en/language.oop5.interfaces.php#example-208 Loosely related to: * squizlabs/PHP_CodeSniffer#2128
…multiple interfaces Interfaces can extend multiple other interfaces. Looking at the sniff code, no changes are needed in the sniff to account for this, however, adding an additional unit test to warrant against sniff changes in the future which may not take this into account. N.B.: I've verified all other usages of the `T_INTERFACE` token in the whole of WPCS and this was the only one which looked at `extends/implements`, so the only one for which interfaces extending multiple other interfaces could be problematic. See: * http://php.net/manual/en/language.oop5.interfaces.php#example-208 Loosely related to: * squizlabs/PHP_CodeSniffer#2128
…multiple interfaces Interfaces can extend multiple other interfaces. Looking at the sniff code, no changes are needed in the sniff to account for this, however, adding an additional unit test to warrant against sniff changes in the future which may not take this into account. N.B.: I've verified all other usages of the `T_INTERFACE` token in the whole of WPCS and this was the only one which looked at `extends/implements`, so the only one for which interfaces extending multiple other interfaces could be problematic. See: * http://php.net/manual/en/language.oop5.interfaces.php#example-208 Loosely related to: * squizlabs/PHP_CodeSniffer#2128
…multiple interfaces Interfaces can extend multiple other interfaces. Looking at the sniff code, no changes are needed in the sniff to account for this, however, adding an additional unit test to warrant against sniff changes in the future which may not take this into account. N.B.: I've verified all other usages of the `T_INTERFACE` token in the whole of WPCS and this was the only one which looked at `extends/implements`, so the only one for which interfaces extending multiple other interfaces could be problematic. See: * http://php.net/manual/en/language.oop5.interfaces.php#example-208 Loosely related to: * squizlabs/PHP_CodeSniffer#2128
|
If the utility method refactor will in fact become PHPCS 3.5.0, this PR should probably be ignored until then and will be addressed and included in the refactor instead. |
|
This has been addressed and published now in PHPCSUtils. For more context, see #2456 (comment) |
As I've just looked at the
findExtendedClassName()method again for PR #2127, I remembered that @SenseException reported another issue with it a while back via Twitter: https://twitter.com/SenseException/status/997988691468484609.Summary: interfaces can extend more than one interface (in contrast to classes) and the
findExtendedClassName()method does not allow for that.See: http://php.net/manual/en/language.oop5.interfaces.php#example-208
To allow for this from within the existing
findExtendedClassName()method would change the function signature from@return string|falseto@return string|array|false, thus breaking the API.So, instead, I've elected to add a new
File::findExtendedInterfaceNames()method and to recommend using that method for interfaces in the documentation of thefindExtendedClassName()method .As the logic needed for these two methods, as well as for the
findImplementedInterfaces()method, is basically the same, I've extracted the logic out to a privateFile::examineObjectDeclarationSignature()method which is now called by all three functions to DRY out the code.Includes dedicated unit tests for the new method.