magento / magento-semver

Magento Semantic Versioning Checker. Join #svc in our Community Slack: https://opensource.magento.com/slack. Static tests that identify required module version changes based on code diff, and look for backward incompatible changes.
Other
32 stars 25 forks source link

False positive - Method return typing changed when class in the same namespace #43

Closed ihor-sviziev closed 4 years ago

ihor-sviziev commented 4 years ago

I've seen few times when SVC check was complaining about changed return type, when actually it wasn't.

Example taken from https://github.com/magento/magento2/pull/27980 here https://github.com/magento/magento2/pull/27980/files#diff-fe16896d208c3ab2d52fccbbf7a5eaebR113:

diff --git a/lib/internal/Magento/Framework/DB/Select.php b/lib/internal/Magento/Framework/DB/Select.php
index 075aa6b24faa..4f79b0d314f1 100644
--- a/lib/internal/Magento/Framework/DB/Select.php
+++ b/lib/internal/Magento/Framework/DB/Select.php
@@ -7,6 +7,7 @@

 use Magento\Framework\App\ResourceConnection;
 use Magento\Framework\DB\Adapter\AdapterInterface;
+use Magento\Framework\DB\Sql\Expression;

 /**
  * Class for SQL SELECT generation and results.
@@ -107,19 +108,19 @@ public function __construct(
      * </code>
      *
      * @param string $cond The WHERE condition.
-     * @param string|array|null $value OPTIONAL An optional single or array value to quote into the condition.
-     * @param string|int|null $type OPTIONAL The type of the given value
-     * @return \Magento\Framework\DB\Select
+     * @param array|null|int|string|float|Expression|Select|\DateTimeInterface $value The value to quote.
+     * @param int|string|null $type OPTIONAL SQL datatype of the given value e.g. Zend_Db::FLOAT_TYPE or "INT"
+     * @return Select
      */
     public function where($cond, $value = null, $type = null)
     {
         if ($value === null && $type === null) {
             $value = '';
-        } elseif ($type == self::TYPE_CONDITION) {
+        } elseif ((string)$type === self::TYPE_CONDITION) {
             $type = null;
         }
         if (is_array($value)) {
-            $cond = $this->getConnection()->quoteInto($cond, $value);
+            $cond = $this->getConnection()->quoteInto($cond, $value, $type);
             $value = null;
         }
         return parent::where($cond, $value, $type);

Actual result

Result is following: image

Level Target/Location Code/Reason
MAJOR Magento\Framework\DB\Select::where/lib/internal/Magento/Framework/DB/Select.php:115 M120 [public] Method return typing changed.

Expected result

Return type isn't changed.

Test complaining that return type has changed, but it actually not, as class Magento\Framework\DB\Select actually is THE SAME class as Select as it in the current namespace.

Additional examples: https://github.com/magento/magento2/pull/27129

https://github.com/magento/magento2/pull/26355#issuecomment-578045167

m2-assistant[bot] commented 4 years ago

Hi @ihor-sviziev. Thank you for your report. To help us process this issue please make sure that you provided sufficient information.

Please, add a comment to assign the issue: @magento I am working on this


okorshenko commented 4 years ago

Hi @ihor-sviziev Looks like this issue was fixed in https://github.com/magento/magento-semver/pull/51

Could you please let us know if this issue is still relevant?

ihor-sviziev commented 4 years ago

Yeah, looks like it was already fixed.