Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@mixin to support behaviors/mixins #35

Closed
samdark opened this issue Nov 18, 2013 · 26 comments
Closed

@mixin to support behaviors/mixins #35

samdark opened this issue Nov 18, 2013 · 26 comments
Assignees

Comments

@samdark
Copy link

samdark commented Nov 18, 2013

Recent PhpStorm EAP supports @mixin. It's used the same way as “use trait” except that target can be any class.

Behaviors/mixins are used in CakePhp, Symfony, Propel, Nette, Yii and Doctrine so it's quite common. Would be great if these will be documented.

Here's the IDE issue history: http://youtrack.jetbrains.com/issue/WI-1730

@mindplay-dk
Copy link

Yep, this is missing.

@mvriel
Copy link
Member

mvriel commented Jan 1, 2014

I have discussed this item with @ashnazg and we have come to the conclusion that the added benefit is unclear and whether it weighs up to adding a new tag. Due to this I have come to the conclusion not to include this tag to the PSR at this time.

@mvriel mvriel closed this as completed Jan 1, 2014
@mindplay-dk
Copy link

The added benefit is extremely clear to me - I am already using it on production code with PhpStorm, and it makes mix-in behaviors so much easier to work with.

Previously, every piece of code that works with types that use mix-in behaviors, had be explicitly documented, inline, on a case-by-case basis, like this:

public function workWith(SomeEntity $entity) {
    /** @var SomeEntity|SomeBehavior|SomeOtherBehavior $entity */
    $entity->someBehaviorSpecificMethod();
}

Imagine having hundreds of methods where you need to document the mix-ins explicitly in every single method.

Now forget all of that and simply document the behaviors once, for the type itself:

/**
 * @mixin SomeBehavior
 * @mixin SomeOtherBehavior
 */
class SomeEntity {
    ...
}

It makes a lot of sense, and I was able to scrap hundreds of redundant inline documentation tags, while still providing the exact same information. There is a lot of value to being able to document things at the root, where they were actually defined, rather than having to document repeatedly and sporadically throughout loosely related code.

If you don't see the benefit, you probably haven't worked with mix-in behaviors?

As @samdark pointed out, lots of mainstream PHP frameworks use them, which means lots of developers need them - which means this tag can save a ton of work for us. That's a clear benefit.

@barryvdh
Copy link

barryvdh commented Aug 1, 2014

This is actually pretty useful, is something similar implemented? Or can this be reviewed?

For example, take this Manager class from Laravel: https://github.com/laravel/framework/blob/4.2/src/Illuminate/Support/Manager.php

That uses the magic __call() method to redirect all calls to the driver. But the IDE doesn't know that. So if we could use the @mixin StoreInterface, it would be a lot easier then adding every single method using a @method tag.
This has been used on multiple classes (query builders, models, drivers/managers etc)

@mindplay-dk
Copy link

It definitely should be implemented - prior to traits in php 5.4.0, the only option for mix-ins was run-time hocus-pocus, which many frameworks implement in many different varieties, most of which could be documented (and already can in e.g. PhpStorm) using this tag.

Since it's already proven useful and already in use, I think you would need a very clear reason not to support it - e.g. there would have to be something wrong with it. There is a clear benefit to somebody (myself included) or it wouldn't have been implemented.

I think the name and syntax of the existing tag is fine, it's behavior is pretty easy to understand, and I don't see any shortcomings or reasons not to support it?

@mvriel mvriel reopened this Aug 16, 2014
@mvriel
Copy link
Member

mvriel commented Aug 16, 2014

Thanks for providing the additional information; I have reopened this issue so that this can be re-reviewed

@barryvdh
Copy link

Should there be distinction between just calling the methods (eg. extending multiple classes) and using __callStatic to go from static to the actual instance.
The first instance is similar to using traits (in php <5.4) and the second is the 'Facade' in Laravel.
Does something like @mixin static StoreInterface make sense?

@Anahkiasen
Copy link

👍 for implementation of @mixin. It's also very useful in the context of tools like PHPSpec which describe classes while making it available via $this. Using @mixin allows to get automatic completion from the class to the spec

@barryvdh
Copy link

So this is another good example, the Laravel Database Model: https://github.com/laravel/framework/blob/v5.0.16/src/Illuminate/Database/Eloquent/Model.php#L3307-L3338

Calls on the model are redirected to a query instance. Using @mixin would complete the functions from the Builder:

@mixin \Illuminate\Database\Eloquent\Builder

But because it's also used statically, it should also be static. So perhaps this?

@mixin static \Illuminate\Database\Eloquent\Builder

@mindplay-dk
Copy link

I think that's a really bad example - what Laravel is doing here is extremely unusual: run-time hocus pocus to make instance members of one class appear to be static members of another class. Delegating from static to instance (or instance to static) members is an extremely unusual and exotic thing - correct me if I'm wrong, but I don't think I have ever seen any other framework but Laravel doing that. (?)

Under any circumstances, an annotation that delegates static to instance (or instance to static) would have some really strange implications in practice, since classes can have a mix of static and instance members... for starters, what happens to members that are already static? Do they get mixed in or not? You can't say for sure - it isn't even something you can know without running the code. Trying to define this in a declarative, static manner doesn't really make any sense at all.

@ababkov
Copy link

ababkov commented Apr 3, 2015

Removed my last post - missed some of the context of the thread.

@Anahkiasen
Copy link

@mindplay-dk Is it the role of PHPDoc to dictate how code should be structured though? To me it should allow you to properly document any kind of code, up to old legacy code doing weird things or edge cases, testing specs etc. The purpose of this PSR is to standardise what has been done in the wild for years now – if @mixin is part of that and is commonly used, which is the case, to me it should be part of the PSR

@mindplay-dk
Copy link

@Anahkiasen I'm not arguing aganist @mixin, I was arguing against @mixin static which (as I understood it) was intended to document the so-called "facade pattern" used in Laravel, where static method-calls are delegated to instance methods. This is, first of all, something very specific to Laravel - it isn't a common pattern, I've actually never seen it used anywhere else. But moreover, the behavior of such an annotation would be hard to define - see the second paragraph of my last response.

@ababkov
Copy link

ababkov commented Apr 6, 2015

@mindplay-dk Wouldn't the "static" behaviour just be that all methods included in the mixin could be called statically or as instance methods .. as opposed to omitting the flag which would result in just instance methods being available?

@mvriel
Copy link
Member

mvriel commented May 13, 2015

For the record: I have completely changed my mind about the validity of @mixin and think it would make a good addition. The exact definition needs to be reviewed but I am quite interested how phpStorm handles this

@mindplay-dk
Copy link

Wouldn't the "static" behaviour just be that all methods included in the mixin could be called statically or as instance methods .. as opposed to omitting the flag which would result in just instance methods being available?

That's just one option - the problem is, there are any number of possible patterns to delegate calls at run-time, we can't possibly predict or support them all. For example, you could implement callStatic() and delegate static method calls to other static methods, or even to different objects, or you could implement __call() and delegate instance method-calls to static methods - I don't see how we can predict or support all of those, but do feel free to propose something.

I started writing up the spec for @mixin as a plain mixin, e.g. instance-methods delegated to instance-methods of another class at run-time, but then something else occured to me. We now have traits, which are extremely close to "real" mixins, and in almost any case are going to be preferable to run-time delegation of method-calls.

Should we even concern ourselves with mixins still?

I know that e.g. Yii still uses them, but is that just a "phase"? Until PHP with trait support is generally available with no more need for run-time tricks?

If we consider this obsolete practice (and I'm not saying we do, I'm asking!) do we want to support or encourage that?

@MarkVaughn
Copy link

MarkVaughn commented Dec 15, 2016

Another reason mixin is useful is from the trait's perspective.

I have a base TestCase all my tests extend from. I have traits for these testcases like Elasticsearch, ModelGenerators, DatabaseMigration, etc. that I mix and match to apply to my test cases. Now from within the trait I use methods and properties provided within the parent class I expect to use with this trait (assertions, faker, application, db connections etc..)
👍 for mixins

@brad-jones
Copy link

brad-jones commented Dec 21, 2016

I would like to see something like @mixin become part of the standard.

See: https://github.com/php-integrator/atom-autocompletion/issues/86

@ghost
Copy link

ghost commented Dec 22, 2016

@MarkVaughn Interesting point, but can't you already solve this by declaring an abstract method inside your trait? That gives you autocompletion and proper checking in PHP itself without the need for a new tag:

<?php

class A
{
   public function foo() { echo "Hello"; }
}

trait T
{
   abstract public function foo(); // Add this to match the expected methods.
   public function bar() { echo $this->foo() . " World"; }
}

class B extends A
{
    use T;
}

$b = new B();
$b->bar();

If you were to use the trait in another class where you forgot to add the method, you may not notice the error until later as there is no checking on a mixin tag. By using an abstract method, you clearly document that your trait requires additional functionality from the class using it.

Also: I don't intend to provide any positive or negative feedback on the @mixin tag, but am just providing a possible available solution for @MarkVaughn's request :-).

@Anahkiasen
Copy link

@Gert-dev That's fine for a few methods but if you access more methods from the base class than that then you constantly need to keep the signatures up to date and it adds a lot of useless boilerplate that is only there for the sake of IDE autocompletion

@ghost
Copy link

ghost commented Dec 22, 2016

@Anahkiasen That is true, it does require extra maintenance from the developer.

I've been thinking about this for a while and, even though I agree that @mixin will be able to solve your problem, I don't feel it is the proper way and thus went back to the original problem you're trying to solve: you need a trait that can use things from the class it is being used in, without explicitly repeating all the necessary functionality solely because a trait is independent from the class it is used in.

To rephrase: you need a trait that can somehow only be used by classes that expose certain functionality and that can assume that functionality is present. That sounds an awful lot like what an interface does for classes. Would it thus not be even more interesting if you could somehow indicate that a trait can only be used by classes implementing a specific interface?

interface I
{
    public function foo();
}

class B implements I
{
    public function foo() { }
}

class C extends B
{
   use T;
}

/**
 * @requires I
 */
trait T
{
    public function bar()
    {
        $this->foo(); // foo is autocompleted since T can only be used in classes implementing I.
    }
}

You'd get everything you're looking for:

  • The method signatures are in one place for both the base class and trait: in the interface.
  • The trait will get autocompletion and things such as refactoring could also work if the refactoring tool support the annotation.
  • Something like this could even be interesting for PHP itself in the future, which would reintroduce the compile-time checking you get from defining the abstract methods, without the hassle (trait T requires I).

The downside is that it would not be possible to access class properties directly, but you can fix that by placing a getter in the interface instead. As a bonus, because of this encapsulation, that also allows other classes to retrieve this information from another source than their properties instead.

@Fleshgrinder
Copy link

An interface is just a fully abstract class, restricting this feature to interfaces only thus makes no sense since there is no difference between interfaces and (abstract) classes after all from the viewpoint of a trait at this point.

I requested a similar feature for PhpDoc and PHP. Facebook's Hack has this ability built-in btw. However, internals do not seem to care and see too many problems with supporting this feature.

The @mixin proposal seems completely fine to me as is. Especially considering that various vendors already have support for it.

@mindplay-dk
Copy link

there is no difference between interfaces and (abstract) classes after all from the viewpoint of a trait at this point

There is one difference though - you cannot type-check against traits, but you can type-check against an abstract base-class, so in that sense there is an important distinction: interfaces and classes are types, and traits are not; they're an implementation detail of which client code is always, by definition, unaware.

That said, I agree that @mixin as proposed is fine. Yes, there are frameworks that compose capabilities at run-time via __get(), __set() and __call(), but they don't strictly have to anymore, and as the years pass, there is actually little excuse not to move away from run-time hocus-pocus to traits and interfaces and proper abstraction - other than backwards-compatibility of projects that are already heavily reliant on custom run-time composition facilities.

I would once again favor modern PHP rather than favoring "clever" frameworks that managed to implement their own "traits" and "behaviors" prior to actual traits. (and, yes, I understand that __get(), __set() and __call() are still language features that should be taken into account, but there are actually "clean" ways to use those, e.g. using them only in cases where argument/return-types of all dynamic fields/methods are consistent - we have better language-facilities like traits to support other dynamic cases today.)

@alexbowers
Copy link

Hi. @mvriel. Any chance this can be looked at again?

@ashnazg
Copy link
Member

ashnazg commented Oct 14, 2018

Ping @ondrejmirtes @muglug @neuro159 @mindplay-dk @GaryJones @mvriel @jaapio for opinions.

Can folks chime in now on what in-the-wild implementations there are for this? Has it already converged into a single syntax that's utilized the same across implementations?

@ashnazg ashnazg self-assigned this Oct 14, 2018
@ashnazg
Copy link
Member

ashnazg commented Oct 15, 2018

If this needs to be pursued further, please bring this up as a new thread on the FIG mailing list for discussion -- https://groups.google.com/forum/#!forum/php-fig

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests