Allow Macroables to override defined methods

[joelmellon]

I'd like to be able to override defined methods on Macroables.

An example would be overriding the Str::start() method to accept an array of values that can "start" a string, and a default if the string doesn't already start with one of the possibilities in the array. eg. I could do:

Str::start('+200', ['+', '-']); // outputs '+200'
Str::start('-200', ['+', '-']); // outputs '-200'
Str::start('200', ['+', '-'], '+'); // outputs '+200' (using the value of the third argument as the "default")

Currently, if I declare a macro like so:

Str::macro('start', function($string, $possibilities, $default = null){
    return 'my custom +/- functionality';
});

The macro closure doesn't get executed when I call Str::start('whatever');. This is because the Macroable::__call() and Macroable::__callStatic() magic methods aren't called when accessible methods already exist with that name. So we don't get a chance to check for macros before out-of-the-box methods are executed. Rephrased: since it's an actual method, calling Str::start() doesn't call Macroable::__call() or Macroable::__callStatic().

One option would be to declare all Str methods as macros out-of-the-box, but that would involve rewrites for all classes that use the Macroable trait.

I'm happy to make a PR but I guess I need a little inspiration. Any ideas other ways to achieve this functionality without resorting to a rewrite of all public methods of any Macroable classes?

[henzeb]

The Facades is also using __callStatic. So a check could be done there, or a proxy method could be added. Instead of $instance->$method(...) you could see if the instance is using Macroable and if so execute $instance->__macroCall($method, ...) which does the actual logic to determine which to execute.

The problem may lay in the expectations. if I use a function called lowercase to do lowercase, I do not want it to transform it from pascalcase to snakecase. Your example could be a valid usecase but you cannot disallow others without doing extensive inspections, which may or may not impact overall performance.

basically, I like, but I also think it's a bad idea. Macroable was created to add functionality to classes from third parties, not to modify it's behavior.

[joelmellon]

There isn't a facade for Str. Instead there’s a top-level class alias for Str which is what developers are using in their code when they write \Str::whatever() or if they use Str;.

Macroable was created to add functionality to classes from third parties, not to modify it's behavior.

That's a pretty strange assumption there. If it wasn't created to "modify it's behavior" then why isn't Illuminate\Support\Str declared as final? Suggesting that Laravel shouldn't be extended is ridiculous. It's a concept that goes way back in its history. It is in fact a core concept that its authors have gone to great lengths to make possible in a vast variety of ways with various mechanisms including: IoC container binding, aliases, facades, providers, interfaces which are in place to help with extension compatibility, factories/managers, events, traits, macros of course, form requests, boot and register methods on various classes...most object-oriented design patterns employed by Laravel facilitate "modification" not just "adding functionality".

[joelmellon]

You have no idea what you're talking about. Go away.

[henzeb]

I'm sorry if my opinion hurt your feelings.

[joelmellon]

What a loser. Troll harder.

[therecluse26]

Lol so salty

[Sophist-UK]

@joelmellon Your comments above are against the Code of Conduct for this repo which state:

• Participants will be tolerant of opposing views.
• Participants must ensure that their language and actions are free of personal attacks and disparaging personal remarks.
• When interpreting the words and actions of others, participants should always assume good intentions.
• Behavior that can be reasonably considered harassment will not be tolerated.

If you don't like what other people are saying or don't like people that disagree with you, please keep your feelings to yourself.

[joelmellon]

My current solution works quite well, although it doesn't use macros. It's a more traditional object oriented approach to extension.

I modified the Str alias in config/app.php to point to my own Str class that extends the Illuminate Str class. This is the best way I currently know how to "override" Str methods. This covers most usage, but not if developers use Illuminate\Support\Str; directly. From working on various projects, it’s quite common for developers to do that, so if I want to override a method or two I need to go through the whole project and replace all occurrences of use Illuminate\Support\Str;. It also doesn’t cover using the Str class alias before Laravel boots, eg. in a config file, so developers need to make sure to use the extended the Str class that I created. The Str alias isn't registered before the app is booted anyway, so the same limitation is true for macros as well.

Maybe this solution is as good or better than using macros to override methods (if that were possible). It would still be nice to be able to override macros; it would be more plug-and-play.

[Sophist-UK]

@jose123v

a. Everyone starts as a noob and learns as they go. If they get things factually wrong, you can still correct them without resorting to abusive comments.

b. The whole point of being a noob is that you are likely to learn and have a better future - so your comment about their future is not only illogical, but is also against the Code of Conduct for this repo. If you don't like what other people are saying or don't like people that disagree with you, please keep your feelings to yourself.

Edit: Before calling anyone "a noob" it might actually be sensible to compare Contribution counts to make sure that it isn't actually you that is the relative noob. LoL

[jose123v]

Not even the moderators care and you are suffering so much, crystal generation 😄😄😄
If you don't like what other people are saying, please keep your feelings to yourself.

[therecluse26]

I'd personally rather laugh at these people than have them get the ban hammer. Open source has a long, storied tradition of bull-headed flame wars, it's our heritage.

[joelmellon]

...and who is a noob with no future.

Who's the noob? lol

[Sophist-UK]

Not @henzeb - his profile has a pretty decent contribution record.

[Sophist-UK]

@taylorotwell Can moderators please act so that this repo doesn't become an unpleasant place to contribute?

[joelmellon]

lol, I'm the reason you're not going to contribute? This thread was buried in time until a hall monitor strolled by.

[Sophist-UK]

This thread was buried in time until an off-topic email entered my email box earlier today. At which point I got annoyed by the tone of the thread and said something appropriate. I would rather not feel the need to be "a hall monitor" but when you have immature bullies throwing insults in the corridor, someone has to say something to stop the entire school becoming toxic.

[joelmellon]

Glad you recognize what you are. I like when you called for daddy's help.

[taylorotwell]

Hey all - this thread has gotten a bit out of hand. We have to keep Laravel a chill, cool place for everyone to talk and contribute. Have instituted a temporary block on a couple users to let things cool down.

[parallels999]

it would also have been great to select the correct answer to avoid confusion

[Sophist-UK]

To answer the original question, I think that the two options (kinda) already discussed are equally valid, with the final choice depending on whether you believe that this functionality would ever be included in Laravel or not:

• [Your recommended solution] Extending the class and pointing the Helper to the new version - the potential issue with this is that there are potentially hundreds of places which already use Str::start which now call your new version, and the stack trace if a bug in the underlying Str causes an exception might raise a couple of eyebrows - but so long as the calls are 100% backwards compatible I don't see any real issues with this. Your use case seems to be fully-backwards compatible to me, and the benefit of this solution is that if the Laravel version ever gets this functionality, there are no changes needed to your code to remove your own extension. The only thing you have to watch with this is the autoLoader being told to revert to the previous version on an upgrade.
• Using a macro but calling it something similar but different e.g. Str::startMulti for your own use - if the base version ever gets extended, then you would have to change your own code. IMO this is the best solution if you don't think the base functionality will ever be extended to include this.

[benbjurstrom]

This would be really useful for re-implementing UUID v7 for those who want it.

    public function boot()
    {
        Str::macro('orderedUuid', function () {
            return Uuid::uuid7();
        });
    }

#45546