[Proposal] Use Macroable trait on eloquent models

[imanghafoori1]

The fact that laravel packages (or laravel modules in other words) can not introduce new relations on the models that exist in the other modules *without touching their code) creates coupling problems.

For example if we have a commenting package which has a Comment model and some migrations to hold comments in a comments table.

when installed there is no way, except to expose a trait to put on the User model to reflect the relation between Comments and User. Ok this is not the end of the world, we can live with it.
We use the trait and $user->comments would be accessible.

But, when we add an other package to help us have like and dislike on the Comment model.
then how to put the trait on a model which already exists within the vendor folder ?!

From the theoretical point of view, this is violating the open-close principle.

For example, when you start to modularize your application with :
https://github.com/nWidart/laravel-modules
you immediately realize that models in different modules can not be totally decoupled.
When you add a add a comment module : the User module code should change to define new relations.
then when you add like module both the comment and user module code should be modified.

[imanghafoori1]

The problem is that the eloquent models support dynamic where clauses and __call() is already in use.
But that can be addressed. it is not a huge dilemma, I think.

[martinbean]

@imanghafoori1 In this instance, I’d expect a Comment model to be created in the application, then extends a package-provided model but then overrides/adds the methods I need.

I do this in an application I develop with Cashier’s Subscription model. I define a Subscription model in my application, extend Cashier’s, and then add a relation to plans that’s missing from Cashier’s version. If this were a comment model, I’d also be able to add traits to enable liking/disliking.

[imanghafoori1]

This does not solve the problem, does it ?! The goal is not to do that. Let me shed some light on it.
when we install the comment package, we do not want to put a trait on the User model to define a hasMany('Comment'... relation.

If you think about the tables. The users table does not need to change and is completely ignorant of the newly created comments table. (Only the comments table contains the user_id) So the comments table can come and go without the users table even realizing it.

We need almost the same thing for the models of these two tables.
Put this, within the comments package :
User::hasMany('comments', Comment::class, 'user_id');
then:
$user->comments will work.

Here the Comments package know and depend upon the user but user does not know anything or depend by any means upon the Comments

[martinbean]

when we install the comment package, we do not want to put a trait on the User model to define a relation

@imanghafoori1 Then how is your User model ever going to know it’s related to comments if you don’t define a relation? This is why packages like Cashier make you add a trait to your User model.

Put this, within the comments package: User::hasMany('comments', Comment::class, 'user_id');

I would not install a package that is modifying my application’s classes at runtime. What if my model isn’t called User? What if it isn’t in the App namespace like a fresh Laravel application?

You should always be in control of what a package is doing from your application. This is why things like service providers exist. A package should not be making assumptions on how things are structured in your application and what they’re called, and then modifying the behaviour of your application at runtime simply because you’ve installed it.

[imanghafoori1]

I am simplifying the example. I have thought about the idea for months and found all the answers, but it is not possible to describe everything here all at once.

What if my model isn’t called User?

You can put the full namespace of the User class in a config file of the comments package. laravel also does it in the auth.php

Then how is your User model ever going to know it’s related to comments if you don’t define a relation?

You define it in the comments service provider boot method.
User::hasMany('comments', Comment::class, 'user_id');

The name of the relation can also be configurable.

Also you may take a look at the way october cms has done this very nicely.

[michaeldyrynda]

Perhaps it best if you submit a PR to the framework with your proposal.

Seems like much discussion isn’t needed here.

If you have a good solution to this problem that will work in most situations, then let the code and accompanying PR speak for itself.

[imanghafoori1]

@michaeldyrynda Well, that doesn't work. The community should understand the need and vote for it.
Anyway I try to experiment with my thoughts.

If you look for examples you can take a look at october cms plugins.

[michaeldyrynda]

Well, I’m against the idea of a packaged modifying my app at run time like you’re suggesting. I’d much rather opt-in to the behaviour explicitly.

Sounds like your implementation is application-specific where you control both the app and the package; that’s cool, but implementing this in a useful way for the wider community I’d be interested in seeing.

Sounds like there’d be a lot of config just so you avoid using a trait to compose the desired behaviour - this is making assumptions about my app, namespaces, class names, etc.

[imanghafoori1]

@michaeldyrynda

Well, I’m against the idea of a packaged modifying my app at run time like you’re suggesting. I’d much rather opt-in to the behaviour explicitly.

It is not a modification of the app. laravel extensively uses the macroable trait. Is it a bad thing? The fact that a model can introduce a method on an other model does not mean that it affects the behavior of the other methods.
We need more decoupling. it's just that.

And let me mention that new modern languages like Go or Rust allow adding new methods to a type at language level.
You should define an struct and then add methods on it.
In other words, all the types are Macroable by default in those languages. In php we need some work around.

I do not want to copy/paste go-lang code here. You may google oop in go and see oop the concepts.

Do you think that the creators of those languages didn't know what they where doing ?! or there was a good reason for such a design?

[martinbean]

It is not a modification of the app. laravel extensively uses the macroable trait. Is it a bad thing?

@imanghafoori1 No, but Laravel uses the macroable trait so you can modify the framework; not so the framework or third-party packages have the ability to modify your application’s first-party code.

The fact that a model can introduce a method on an other model does not mean that it affects the behavior of the other methods.

I beg to differ. If a model is adding methods to other models in my application at runtime, then that can produce undesirable side effects if it’s overriding an already-defined method, and cause difficult-to-diagnose bugs that happen only at runtime.

And let me mention that new modern languages like Go or Rust allow adding new methods to a type at language level.
You should define an struct and then add methods on it.
In other words, all the types are Macroable by default in those languages. In php we need some work around.

It sounds like you want to write Go or Rust. Have you thought about using those languages if you feel they’re more appropriate?

Do you think that the creators of those languages didn't know what they where doing ?! or there was a good reason for such a design?

Laravel not copying another language’s paradigms is not saying those languages and their creators or wrong or “didn’t know what they [were] doing”. That’s simply a false dichotomy.

[Patryk27]

It's worth adding two cents to this discussion:

And let me mention that new modern languages like Go or Rust allow adding new methods to a type at language level.

Not exactly - at least Rust forbids frivolous implementations: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b82f7fc53c609572b76a3b7cec7928bc.

You can implement straight-away custom methods for types created by yourself and declared in your module only, so your case of having many modules fiddling with other modules' types would fail anyway.

What you can do is create a trait and then implement that trait for any other type (https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=cf35cc82989eeef903c4c7389a61e9f5), but it's a completely different mechanism that does not affect the original type at all.

[imanghafoori1]

@Patryk27 Thank you for your exact investigation.
Of course, rust is by far the safest language of all time.

For eloquent models, whether we are going to affect the original behavior or not...
it depends on how we implement that "macroable" thing exactly.

[imanghafoori1]

I have had this problem in my mind around a couple of months or so.
But I think because this is new to you, it appears scary or somewhat monstrous. (like I was at first.)
Let the idea be in your mind for while, your brain will digest it in a background thread. 😉

[imanghafoori1]

After all, it just boils down not to violate the Liskov's substitution principle and the theory of types.

[imanghafoori1]

After reviewing the L in SOLID I found that adding a method to a type is proven to be harmless in theory,
as long as invoking that method does not weaken the invariants of that type, which does not seem to be the case for eloquent objects.

The problem can occur when we override a method. Overriding consists of
1- remove the current method
and
2- adding a new method with the same name.

The problem actually comes from removing .
So since the macroble trait does not allow overriding the host class methods, it is safe to be used.
and the resulting type would be a subtype.

[martinbean]

Harmless in theory, but not in practice.

Packages should not be modifying client code in the same way the framework doesn’t. Yes, the components Laravel offer are macro-able, but that’s so the client (first party) can add methods to framework classes, and not as a mechanism for third-party code vendors to add methods to first-party classes.

You can quote academic texts as much as you want, but it doesn’t mean a mechanism for third-party packages to override code in an application any better. In fact, it makes it an attack vector for third parties if they can take over a popular package, add macros that add methods that collect data, or destroy databases, or some other nefarious action.

[imanghafoori1]

Packages should not be modifying client code

Agree, but as I just said it is not a modification, it is just adding. so the current behavior remains unchanged.

What I talked about was safety against run time errors. attack vector is a security issue.

Currently any package can setup a listener DB::listen(...) to steal data or execute a raw sql to drop the whole database.
The Schema facade also gives a list of tables to any third party package.
Open source php is not like a C++ compiled binary, we can see what the package is doing.

[imanghafoori1]

instead of putting a new method on the User Model

class CommentingPackageServiceProvider
{
  public function boot  () {

      Comment::belongsTo(User::class, 'user');
      User::hasMany(Comment::class, 'comments');


      // Or to be flexible and configurable
      // Will return an string like :  '\App\Models\User'
      $userClass = config('commenting.user_model'); 

      Comment::belongsTo($userClass, 'user');
      $userClass::hasMany(Comment::class, 'comments');
  }
}

So when the commenting package (module) gets installed or uninstalled the user package (module) does not need to change at all.

[rs-sliske]

To me this feels the package should just have a trait, which users of the package can add to the model they want.

[imanghafoori1]

@rs-sliske I know adding traits is the current habit of the community.
But that has problems which are discussed above.

[donnysim]

Won't you end up coupling them anyway? Sure the module will register a relation like Comments on User, but if you're creating a comment for the user or getting user comments, you're already coupling them. Remove comments module and everything still breaks and you have to go fix it. Auto register or not, you're still going to modify parts of the code depending on module existence.

[imanghafoori1]

@donnysim Not actually, usage of the $user->comments()->... is only within the comments module. (Not the app folder or UserModule).

Only the CommentsController within CommentsModule uses $user->comments()->... plus the modules which depend on the Comments Module. like LikableComments module.

So if we uninstall the Comments module all the usages of those relations go away with it.

The user module does not know whether a commeting system is installed or not.

The thing I am suggesting is already working for a long time in october CMS without any problem.

[donnysim]

Not actually, usage of the $user->comments()->... is only within the comments module. (Not the app folder or UserModule).

That's assuming the comments will only be used with the user. You won't be using your CommentsController within CommentsModule when for example you're displaying Task comments. I don't see any point in having a module that's specific to another module - just have it in one module as opt-in thing. In this case CommentsModule would have a morph relation if you want to use it on in any other module, otherwise it already is dependent on specific module.

Only the CommentsController within CommentsModule uses $user->comments()->... plus the modules which depend on the Comments Module. like LikableComments module.

That's the problem - the modules which depend. Remove the module and your app breaks. Every module will depend on the CommentsModule if you need to display the comments in more optimal way (eager loading, etc.). How do you imagine you will pass comments to a blade template without depending on CommentsModule module?

[imanghafoori1]

@donnysim You are totally right and I agree with you,
The main application after all depends upon all the modules it uses.

But we are trying to decouple the modules used by the main application.
to give package developers more opportunities.

if you provide a commeting package and then provide an other optional package to put like on those comments. you can not give like traits to put on the comments model which live in the vendor folder.

[martinbean]

if you provide a [commenting] package and then provide an other optional package to put like on those comments. you can not give like traits to put on the comments model which live in the vendor folder.

So if I install a couple a packages that are automatically adding methods to classes in both my application and other packages (i.e. adding likes to comments) then I’m quickly not going to know what the hell’s going on in my application because packages are running amok adding methods left, right, and centre.

[imanghafoori1]

@martinbean I know, You are right, that danger exists anyway.
And the same thing already exists for the IOC container.

Currently, When you install a package ( A ), that can bind many keys on the container and override other packages keys and the keys you have bound in your AppServiceProvider for example.

// package A provider
public function boot () {
    app()->bind('yourKey', 'bye bye your value, hello my value ');
}

But this is not the end of the world, package developers are careful enough to choose specific keys to avoid conflicts.
They are not that eager to rush and screw thing up intentionally.

Plus, the relation names can be read from config files to resolve possible conflicts.

Yes, of course, it needs some discipline and the introduced relation names should be declared in the module docs so that package users can understand what is coming from where.

Inevitably when people want to share code, they should reach consensus, and conform to contracts to some level, otherwise chaos will happen.

[imanghafoori1]

This idea is now working as a package

https://github.com/imanghafoori1/eloquent-relativity

[AmirrezaNasiri]

@michaeldyrynda I think there is a useful scenario for having models macroable.

Being able to make models macroable will make our code much more readable in cases like testing environment in which we don't want to populate model with methods only used in testing.

Let me explain the need with an example:
Let say that we're going to test a model named Video and test if it the corresponding source file is being stored or not. So we usually define a method like this and call it in the test case:

private function assertVideoSourceExists($video) { ... }
private function assertMissingVideoSource($video) { ... }

Now let say that we need the assertion in out of a single test case file. In this case, we usually extract assertions in a trait like VideoAssertionTrait or simply put them in TestCase file since test cases extend this one.

The problem comes in when we have more models (like Audio, Music, Cover and etc.) which might be related to each other and they all should be tested in single methods. So in these tests, our files will be full of these kind of method calls which I've actually seen in many projects:

$this->assertMissingVideoSource($video);
// ...
$this->assertAudioSourceExists($extracted_audio);
$this->assertVideoSourceExists($video);
$this->assertCoverImageExists($cover);

I think it would be awesome and much more readable (since readability is important in tests) to do the assertions like this:

$video->assertMissingSource();
// ...
$extracted_audio->assertSourceExists();
$video->assertSourceExists();
$cover->assertImageExists();

Accomplishing this readibility required models to be macroable (again, since we don't want to put test methods in the model itself) and being able to define them like this:

Video::macro('assertMissingSource', function() {
   // assertion logic
})

The reduced code letters might not be that much but saying that a helper function like tap() makes it possible to get rid of temporary variable headaches, I think this can be useful to get rid of those redundant $this and model related letters in method names.

Of course, this is only an example out of other uses of macroable models in testing environment.

[ollieread]

It is my understanding that the whole Macroable setup was created so that developers could extend classes that they otherwise wouldn't have access to. You have access to your models, so you can change them. Making the models macroable will just confuse things.

If you need to provide model related functionality in packages you provide a trait that the user adds to whichever model they want.

If you're adding methods to models, using Macroable, those methods are going to be super magic and super hidden. Unless the developer knows EXACTLY how your package works they're not really going to understand what's happening, are they? IDEs won't pick this stuff up, and since it's entirely dynamic and done at run time it'll be super hard for anyone to track down.

All in all, it's an unnecessary bad idea.

@AmirrezaNasiri You absolutely do not, under any circumstance, want to start introducing unit test functionality into models.

[cf-git]

Such functionality is already there. You add method through the scope, and add the autoload attribute through event if necessary

/** AppServiceProvider */

    public function boot()
    {
        User::addGlobalScope(new class implements Scope {
            /**
             * @param Builder $builder
             */
            public function extend(Builder $builder) {
                $builder->macro("profile", function (Builder $builder) {
                    return $builder->getModel()->hasOne(Profile::class);
                });
            }
        });
        User::first()->profile // return null anyway
        User::with('profile')->first()->profile // return profile if exists
        User::first()->load('profile')->profile // return profile if exists

        // Autoload of profile attribute
        User::retrieved(function (User $model) {
            $model->load('profile');
            $model->makeHidden('profile');
        });
    }

[javoscript]

Seeing that the macroable ability will probably never be added to the core, I made a little package a few days ago to add macros to Eloquent models.

Github repo: Laravel Macroable Models

What do you think of the implementation?

[imanghafoori1]

@martinbean
@michaeldyrynda
you all laughed and said the idea is stupid...
but Taylor included this feature in laravel 7.x
So do not see your self as the most mature and other people's ideas as plagues.

laravel/framework#33025

[ryangjchandler]

@martinbean
@michaeldyrynda
you all laughed and said the idea is stupid...
but Taylor included this feature in laravel 7.x
So do not see your self as the most mature and other people's ideas as plagues.

laravel/framework#33025

Seems like a bit of a toxic reply to be honest...

[imanghafoori1]

Anyway, too much resistance may kill off useful ideas.
I wrote this because I was faced with toxic replies.

closing the issue since the idea is now merged.

[martinbean]

@martinbean
@michaeldyrynda
you all laughed and said the idea is stupid...
but Taylor included this feature in laravel 7.x
So do not see your self as the most mature and other people's ideas as plagues.

laravel/framework#33025

@imanghafoori1 Iman, get rid of that massive chip on your shoulder. No one “laughed” or called you “stupid” at all. We replied with well-reasoned answers as to why your approach was bad. Just because we didn’t agree with your implementation doesn’t mean those replies were “toxic”.

Yes, Taylor’s merged something that does something similar but in a different, safer way. So step off with your, “Ha, I told you so” attitude.