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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.5] Allow macros to be registered using classes (in addition to closures) #19782

Merged
merged 2 commits into from Jun 27, 2017

Conversation

Projects
None yet
2 participants
@sebastiaanluca
Contributor

sebastiaanluca commented Jun 26, 2017

In line with the new validation rule classes and the thought that everything should be in its own class, this draft PR adds the ability to register macros using classes in addition to closures (be it for collection, request, etc).

How it can benefit

A messy service provider like so:

screen shot 2017-06-26 at 20 07 51

Can be reduced to just a few statements giving a better overview:

screen shot 2017-06-26 at 20 09 47

It's also neater to have everything separated so you don't have to go search where you registered a certain macro, it's all there in your project directory overview like any other class.

How to use

Create a macro (for a collection for instance) and have it implement the macro class:

<?php

namespace App\Macros\Collections;

use Illuminate\Contracts\Support\Macro;

class MyMacro implements Macro
{
    public function handle() : callable
    {
        return function ($array) {
            return array_merge($this->items, $array);
        };
    }
}

Bind the macro to a method name like usual:

Collection::macro('myMacro', MyMacro::class);

That's it!

Concerns

Losing the closure completely

Right now it's just weird you have to return a closure inside the handle method. I tried an implementation similar to the one already in place to have the class using the Macroable trait adopt the handle method from the child macro class as-is, but PHP won't let me.

Something like:

$reflection = new ReflectionClass($macro);
$closure = $reflection->getMethod('handle')->getClosure(new $macro);

return call_user_func_array($closure->bindTo($this, static::class), $parameters);

Unfortunately this only works for anonymous functions as it throws an exception when I want to bind MyMacro's method (now a closure) to another class' context. Any idea how to go about this?

Other

  • This is core functionality and can't be extracted to a package, therefore this PR.
  • Does removing the callable typehint from the macro method pose any issues? Perhaps a new method to register a macro class would be better.
  • Need to add/update tests, but would love a 馃憤 or 馃憥 before I put in the work.

Feedback welcome!

sebastiaanluca added some commits Jun 26, 2017

Implement macro class
Signed-off-by: Sebastiaan Luca <hello@sebastiaanluca.com>
Adhere to PHPDoc style guidelines
Signed-off-by: Sebastiaan Luca <hello@sebastiaanluca.com>
@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 26, 2017

Member

But this doesn't work for some of your examples, right? $this will not be bound to the original class, so your transformKeys example will not work.

Member

taylorotwell commented Jun 26, 2017

But this doesn't work for some of your examples, right? $this will not be bound to the original class, so your transformKeys example will not work.

@sebastiaanluca

This comment has been minimized.

Show comment
Hide comment
@sebastiaanluca

sebastiaanluca Jun 27, 2017

Contributor

@taylorotwell Unless I misunderstand, any macro closure will work since the Macro class is just a wrapper returning a closure. Identical to how it's currently done, $this will then refer to the class the macro is bound to (collection, request, route, 鈥). Somewhat dubious, but not different from the current situation where one might think $this refers to the service provider the macro closure is used in. The target class just adopts this closure as its own method.

I wanted it to adopt the handle method to reduce nesting, but that doesn't work and I haven't figured out an alternative yet. Still, I think this is a neat feature that would be useful to many, despite the odd nesting (for which we might find a solution anyhow).

So for the transformKeys example:

Define the macro:

<?php

namespace App\Macros\Collections;

use Illuminate\Contracts\Support\Macro;

class TransformKeys implements Macro
{
    public function handle() : callable
    {
        return function (callable $operation) {
            return collect($this->items)->mapWithKeys(function ($item, $key) use ($operation) {
                return [$operation($key) => $item];
            });
        };
    }
}

Register the macro:

Collection::macro('transformKeys', TransformKeys::class);

Use it:

collect([
    'a' => 1,
    'b' => 2,
    'c' => 3,
])->transformKeys('strtoupper')

// Results in:
//[
//    "A" => 1
//    "B" => 2
//    "C" => 3
// ]

Would be nice to be able to do collect(['a' => 1])->transformKeys->strtoupper, but maybe that's for another PR :)

Contributor

sebastiaanluca commented Jun 27, 2017

@taylorotwell Unless I misunderstand, any macro closure will work since the Macro class is just a wrapper returning a closure. Identical to how it's currently done, $this will then refer to the class the macro is bound to (collection, request, route, 鈥). Somewhat dubious, but not different from the current situation where one might think $this refers to the service provider the macro closure is used in. The target class just adopts this closure as its own method.

I wanted it to adopt the handle method to reduce nesting, but that doesn't work and I haven't figured out an alternative yet. Still, I think this is a neat feature that would be useful to many, despite the odd nesting (for which we might find a solution anyhow).

So for the transformKeys example:

Define the macro:

<?php

namespace App\Macros\Collections;

use Illuminate\Contracts\Support\Macro;

class TransformKeys implements Macro
{
    public function handle() : callable
    {
        return function (callable $operation) {
            return collect($this->items)->mapWithKeys(function ($item, $key) use ($operation) {
                return [$operation($key) => $item];
            });
        };
    }
}

Register the macro:

Collection::macro('transformKeys', TransformKeys::class);

Use it:

collect([
    'a' => 1,
    'b' => 2,
    'c' => 3,
])->transformKeys('strtoupper')

// Results in:
//[
//    "A" => 1
//    "B" => 2
//    "C" => 3
// ]

Would be nice to be able to do collect(['a' => 1])->transformKeys->strtoupper, but maybe that's for another PR :)

@taylorotwell taylorotwell merged commit 7908979 into laravel:master Jun 27, 2017

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Jun 27, 2017

Member

I refactored this into a new mixin method on Macroable. You can do SomeMacroableClass::mixin(new Mixin). All methods on the mixin will be available to the macro'd class.

Member

taylorotwell commented Jun 27, 2017

I refactored this into a new mixin method on Macroable. You can do SomeMacroableClass::mixin(new Mixin). All methods on the mixin will be available to the macro'd class.

@sebastiaanluca

This comment has been minimized.

Show comment
Hide comment
@sebastiaanluca

sebastiaanluca Jun 27, 2017

Contributor

Hadn't thought of that, even better :) Thank you!

Contributor

sebastiaanluca commented Jun 27, 2017

Hadn't thought of that, even better :) Thank you!

@sebastiaanluca

This comment has been minimized.

Show comment
Hide comment
@sebastiaanluca

sebastiaanluca Jun 27, 2017

Contributor

@taylorotwell Trying out some scenarios, noticed a few things that might be of interest to you.

  • A mixin can override any method on the macroable class, regardless of visibility. This allows for lots of use cases and dynamic coding, but probably some unwanted situations too. Then again, it's the user's code, right? They probably know what they're doing.
  • Should protected methods be mixed in as well? My feeling is they're out of the mixin scope (same as private).
  • Mixin can use both mixin class variables (static or instance) as well as variables of the macroable. Might be useful to mention in the docs. (see https://twitter.com/sebastiaanluca/status/879754432778469378)
Contributor

sebastiaanluca commented Jun 27, 2017

@taylorotwell Trying out some scenarios, noticed a few things that might be of interest to you.

  • A mixin can override any method on the macroable class, regardless of visibility. This allows for lots of use cases and dynamic coding, but probably some unwanted situations too. Then again, it's the user's code, right? They probably know what they're doing.
  • Should protected methods be mixed in as well? My feeling is they're out of the mixin scope (same as private).
  • Mixin can use both mixin class variables (static or instance) as well as variables of the macroable. Might be useful to mention in the docs. (see https://twitter.com/sebastiaanluca/status/879754432778469378)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment