Store::getId should return an int

[AntonEvers]

Description

\Magento\Store\Model\Store::getId should return an integer according to the phpdoc.
In reality it returns strings as well.

Fixed Issues (if relevant)

1. magento/magento2#<issue_number>: Issue title
2. ...

Manual testing scenarios

1. ...
2. ...

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[orlangur]

Does it break anything? I believe there are 100500 such places, numeric strings are considered to be int all over the code thanks to PHP loose typing.

Recent string casting example of numeric string just because JavaScript is not so type tolerant: #10475

Also, if such change does make sense, the right place for type casting is a setter, not getter.

[AntonEvers]

Hi @orlangur, good point. The thing is, when the phpdoc says @return string|int, I know that I cannot perform an if ($store->getId()) {} on it but I should be more specific. But if the phpdoc says @return int there should be no problem in using if ($store->getId()) {}.

I think you cannot state that "numeric strings are considered to be int all over the code thanks to PHP loose typing" because 0 == false and "0" == true; the phpdoc should tell you what type is actually returned and not how a human could interpret the type.

[orlangur]

"0" == true this is in JavaScript, every non-empty string is converted to true, not in PHP.

Vlads-MacBook-Pro:orlangur$ php -r 'var_dump("0" == true);'
bool(false)
Vlads-MacBook-Pro:orlangur$ php -r 'var_dump(0 == false);'
bool(true)

So, in PHP you can use if ($store->getId()) {} safely. Declaration @return string|int usually means method can return some integer or some string like 'store_code'. This is misuse of string return type if the method returns only numeric strings as to me.

Try changing \Magento\Framework\Model\AbstractModel::getId to

    public function getId()
    {
        var_dump($this->_getData($this->_idFieldName));
        return $this->_getData($this->_idFieldName);
    }

and open any page - NOT A SINGLE int will appear, all IDs are strings internally.

Thus I don't think such kind of type casting should be introduced in some random places as it does not bring any value but breaks the things as in issue I mentioned and in a bunch of broken by this PR tests.

I really like the idea of strict types and compile-time type checks though. The right way to do this is writing new code using https://wiki.php.net/rfc/scalar_type_hints_v5#strict_types_declare_directive and appropriate return type declarations (and not just PHPDocs + type casting).

[AntonEvers]

I have been mixing up languages apparently. I'm closing this one. But the strict types would indeed be very very nice. Hopefully somewhere along the road. Thanks @orlangur for being sharp.

[ihor-sviziev]

Hi @orlangur,
Issue appears when you're using strict types declare(strict_types=1) in your file. Then you expect that if value should be int - you can use it for your method that has declaration like this

<?php
declare(strict_types=1);

function myFunction(int $storeId): void
{
}

$storeId = '2'; // let's assume we executed $store->getId(); that has defined return type int
myFunction($storeId);

output of php 1.php:

PHP Fatal error:  Uncaught TypeError: Argument 1 passed to myFunction() must be of the type integer, string given, called in /home/ihor/1.php on line 7 and defined in /home/ihor/1.php:3
Stack trace:
#0 /home/ihor/1.php(7): myFunction('2')
#1 {main}
  thrown in /home/ihor/1.php on line 3

In this case you'll get type error. Why - because string !== int.

---

BTW return type for getId method should be int|null -> something like public function getId() ?int