framework: Collection methods don't work with built-in PHP functions

And why is that a framework problem?

I should probably ask why isn’t that a framework problem?

And why is that a framework problem?

@rodrigopedra Let’s just make sure we’re not talking passed one-another. Me voicing my opinion, you voicing yours.

Had the laravel documentation present the Collection.map() in the documentation like this:

collect([])->map(function ($value, $key) {
});

and described it as “function that maps a value and a key to other value”, and also probably written “There is no function to map only the value to another value”. Then you would have all the rights in the world. You would be most kind of correct, and noone could argue with you otherwise. Actually, I would agree with you more than I can say. That function should only work with callables that accept two arguments and should not work with "strtoupper". No question about it.

But that’s not the case.

What Laravel documentation now says is

This is fine. Collection.map() “maps one value to another”. It presents the map() function as a function that works fine with one argument.

collect([])->map(function ($value) {
});

And it works… only if you pass in a function that ignores superfluous arguments, because if you pass a function that doesn’t (like "strtoupper") it throws an error. Why? Because that is a lie. It’s not a function that accepts one argument. It only works, because the documentation and most of the use-cases are filled with anonymous functions which ignore the superfluous arguments. It’s actually fooling its users to believe that the function “has a variable amount of arguments”. If you pass function ($value) {} it accepts one, if you pass function ($value, $key) {} i accepts two. But it’s not a feature of Collection.map(), but of the subset of PHP functions - lambdas.

This trick could only work in languages like PHP and maybe JS, where superfluous arguments are ignored. Other dynamic languages like Python or Ruby throw exceptions for superfluous arguments everytime.

It’s bad design.

In my opinion, the right way to handle it would be:

• Not pass second argument as ->map() (and instead add function like mapEntries())
• Accept Closure instead of callable, then you can pass all the superfluous arguments you want (because all Closure will ignore them, unline some callables)
• Something else.

In other words: Collection.map() appears to accept callable, but when you treat it seriously, it appears that it doesn’t accept them.

Okay. I can rename the issue to “Current interface of the collection method callbacks is only suitable for callbacks which are anonymous functions, but not built-in functions”. That’s actually a good idea.

That’s not accurate, either.

Very rarely does something documented as accepting a callable actually mean “anything that can be called upon in the context of the function it was provided to”. More often than not, those callables are expected to have specific parameters and returns and not matching that expectation will eventually result in something breaking. Just using the helper functions Laravel provides, I wouldn’t expect collect()->map(encrypt(...)); to work correctly without wrapping it in a lambda if I need to change the second parameter as its second parameter expects a boolean value (which means through type coercion you’ll always get a boolean true, which by coincidence matches the default behavior). Likewise, if you wanted to do something like collect()->map(object_get(...));, that’s only going to work if your collection is a list of objects; and when it’s a collection of objects, a callable expecting strings would then not be usable because of type conflicts.

The callback has a callable declaration in the doc blocks nowadays. That’s the best option you have for making sure what you’re passing through matches the callable the map() method requires. Adding runtime validation in for the callable here (or in any of the collection class methods) honestly offers little gain; if the code’s going to error out, PHP will give you something more useful than anything the framework could check.

As for built-in functions erroring out when called with extra arguments compared to userland functions not, take that one up with PHP core. IMO it’s one of those inconsistent behaviors that should be changed one way or the other (probably so the C-defined functions behave the same as userland, as there’d be a lot of bad side effects if userland were changed to behave the same as core).

If the interface of ->map() were that “callback receives a value and the number of left padding” that would be fine.

Well the interface of ->map() is: “callback receive the element and its index”, as stated in Laravel documentation:

https://laravel.com/docs/9.x/collections#method-map

So, by your own criteria, you are the one not adhering to the documented interface. So by your own terms, you are not accepting the issue, this is not a frameworks problem.

There isn’t a usecase for collect()->map(‘str_pad’) (passing index as a padding), doesn’t make sense.

Of course there is:

collect(array_fill(1, 10, 'X'))->map('str_pad')->implode("\n");

This will generate a triangular pattern, which could be used in a CLI program to output cool effects. Due to you not finding a use case, doesn’t mean there is not an use case.

@Danon I am actually the one who thinks you are not understanding the issue.

I won’t reply anymore, I suggest you do either the following:

1. Accept not everything works as we expect and embrace error messages to improve your code
2. Propose a PR to Laravel modifying Collection@map to use PHP’s reflection classes to assess its callback arity and conform to the user expected “interface”
3. Propose a RFC to PHP to change the language behavior when a built-in function is called with parameters in excess, so users are not surprised when they don’t read, or don’t like to read, documentation.

Good luck 😉

It’s frameworks choice how to do things. If it wants to pass 2 arguments - it does, don’t like it don’t use it or do a pull request and see what they think.