Replaces full class name with self

[sreichel]

No description provided.

[tmotyl]

will this still work if you override Mage_Directory_Model_Currency class with custom class?

[sreichel]

Yes, overrides still work ...

<?php

class A
{
    public static function test($class)
    {
        if ($class instanceof self) {
            echo 'yes';
        }
    }
}

class B extends A
{}

A::test(new B());

[tmotyl]

I think the problem will occur when people will override the class and copy some methods to change them, ending up with code:

<?php

class A
{
    public static function test($class)
    {
        if ($class instanceof self) {
            echo 'yes';
        }
    }
}

class B extends A
{
public static function test($class)
    {
        if ($class instanceof self) {
            echo 'yes';
        }
       //some additional code modification
    }
}

B::test(new A());

This will yeld wrong results.
So I'm against changing class name to self in "instanceof".
The logic of instanceof is to check whether sth implements a certain interface. Making it dynamic will cause hard to debug issues.

[sreichel]

Would this break current code? Guess not.

It is just a problem if someone would copy&paste mehtods, w/o thinking about it .... For existing code it should not matter.

[colinmollenhour]

I agree with this change since it does not break anything and it is up to coders who copy and paste to do so correctly, I think that has always been the case. However, I see it as a minor point. @sreichel what is the case for doing this as it relates to the end goal to use PHPCS and PHPStan? If we don't include this change will it always be throwing up warnings/errors?

[sreichel]

@colinmollenhour this is not related to phpCS or phpStan ... it will not throw errors. Just want to have clean code ...

[colinmollenhour]

Ok, seems like a minor subjective issue so I will approve as-is.

[tmotyl]

For me, it's just making people life harder without much benefit.
so +1 for the change in general -1 for the "instanceof" change

[sreichel]

For me, it's just making people life harder without much benefit.

Mhh ... at the other hand it enforces coders to review their copy&paste-code a bit more. I'd leave this open for the next days and more opinios about that. OK?

[tmotyl]

I don't agree with "instanceof" change

[sreichel]

I don't agree with "instanceof" change

As said .... it does not affect current copied code ...

it does not break anything and it is up to coders who copy and paste to do so correctly,

[Flyingmana]

Iam not Approving or Disapproving here, but I want to note two things.

Mhh ... at the other hand it enforces coders to review their copy&paste-code a bit more.

It is not enforcing them to, it is just punishing them for not doing it more

this is not related to phpCS or phpStan ... it will not throw errors. Just want to have clean code ...
It is not clear under which aspect of Clean Code this falls.

Clean Code has a very wide spectrum from "it looks subjectively better this way" to "there is a well described and documented case of why this format/pattern leads to less bugs and easier understanding"

So going through it now myself.

using self for the method and constant access is increasing the extensibility, so definitive worth it.

For the instance of it is in this cases used like an interface, to make sure a certain method exists.
When extending, this behavior should usually be the same.
So what benefit does the `self`` bring. It makes renaming the Class easier. It also makes it easier to copy the code to a new Class (not an extended class), both use cases which I dont see us have.
Even with searching I did not find any other beneficial use case.

Its probably safe to merge, but could throw people off, who are trying to extend this parts in the future. (and we know, Magento Devs will make clear, if the framework is causing issues)
But if this happens, its easy to revert this part, as there are no big benefits getting lost by reverting.

I would like to encourage you all, to feel more likely to split PullRequests up, when only parts lead to discussion, which are easy to do independently.
We do this Reviews because its hard for a single person to know all the side effects a change can have, and many eyes usually help finding them.

Also a few Informations to the context of the changes and links to explainations why certain patterns and styles are useful to apply could help avoid discussions and search for proof of correctness.

[colinmollenhour]

Why is this in draft status?

[sreichel]

Why is this in draft status?

See @Flyingmana comment.

[colinmollenhour]

I feel bad for not knowing but is this covered by any PSR or Magento Coding Standard?

[sreichel]

Guess not. I'll split this PR.

[colinmollenhour]

@sreichel Please fix conflicts. Thanks!