Mage_Api - DOC block update

[sreichel]

Cleaned #677

[Flyingmana]

int was correct, its even castet to (int)

[sreichel]

No, it was not.

$id comes from $rid = Mage::app()->getRequest()->getParam('rid', false); what should be a string.

[colinmollenhour]

This is closely related to an issue that I think is going to cause a ton of headache. The PDO library used by Magento returns all values from the database as strings even when they are numeric columns in the database. So while we all think of things like primary key ids as "int" in PHP because they are represented by an int column in MySQL, PHP is actually operating on string values. So for example:

$product = Mage::getModel('catalog/product')->load(1);
$product2 = Mage::getModel('catalog/product')->load($product->getId());

On the first line, load() is receiving an int but on the second line it is receiving a string. So should the @param for this method be an int or a string?

[Flyingmana]

it should be closest to what is expected, and as it specificly converts the parameter to int $this->getSelect()->where("role_id = ?", (int)$id);

I could extend it a bit to crazy edge cases...
string would maybe also support objects which implement the __toString() method(?), which will cause a notice and a wrong value of (1) here.

[sreichel]

On the first line, load() is receiving an int but on the second line it is receiving a string. So should the @param for this method be an int or a string?

IMHO .... for now the doc blocks should reflect the current state. Even if it is unintentional. So, i would use string here, b/c it either comes from request parameter or db object (not from hardcoded ID).

To fix it "at all" you have to ...

• add setter/getter methods for all entities/properties with correct type casting ... dont ask what time it tooks to just add @method annotaion to all that classes?
• enable strict types to ensure consistent code (we already dropped php5^^)

Even in M2 same properties on several entities were handled different ... magento/magento2#18079

Again ... this PRs should be doc-only fixes. I agree, a lot of things could be "fixed", but its out of scope (of thes PRs)

[sreichel]

Thanks for pushing this, BUT .... i have splitted the complete PR just into different classes and smaller PRs, but they depend on each other. Skipping this line will (maybe) break other static tests ...

If the others are okay with this, i'll update during next days.

[tmotyl]

I think that, to move things forward (and I would love to do it quickly) we need:

1. merge all nobrainer changes to doc comments
2. handle few cases where we need also to fix the code (e.g. add some casting)
3. add static code analysys to github actions

So until we reach point 3 I accept that the static code analysis will still be red.

[colinmollenhour]

I agree with moving ahead and merging these "DOC block update" PRs as-is (barring some actual errors that break execution if there are any) and then figuring out later a long-term solution. It is just much more pleasant to code when all of the methods have proper vardocs and while some of the finer points are debatable (e.g. string vs int), this is a huge step in the right direction and I'm sure it was no small effort.

I used a project recently that used PHP CS-Fixer and it was a very nice experience and super easy to add to Travis.

[tmotyl]

I just dont want the effort we put into the topic to detect places where api is inconsistent (expected type is different the really returned in some cases) to get lost. So I think that at least the issues should be added for such cases if we are gojng to merge this patch as is.

[sreichel]

I just dont want the effort we put into the topic to detect places where api is inconsistent (expected type is different the really returned in some cases) to get lost.

There should not be any inconsistencies. This is what static analysis is for?

It is just much more pleasant to code when all of the methods have proper vardocs and while some of the finer points are debatable (e.g. string vs int), this is a huge step in the right direction and I'm sure it was no small effort.

💯

THANK YOU VERY MUCH!

I have added/fixed thousends DOC blocks, added tons of @methods to make work easier. I have reverted code/logical changes as requested, to make it easier to review. No, really NO, i'll not change minor things as mentioned so far.

Take it or leave it :(

I'll update for conflicts, but no further changes, please.