laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

[Proposal] Add nullable type #1757

Open imanghafoori1 opened 5 years ago

imanghafoori1 commented 5 years ago

There are a lot of cases where the methods on the classes return null to represent a mission value. And the callers of the methods should be careful to handle them.

/**
* @return User|null            <---- consider here.
*/
public function find ($id) {
     $user = TwitterApi::search($id);

     if (!$user) {
         return null;
     }
     return new User($user);   
}

If the caller forgets to check for null and handle it, he may have the infamous null reference error at run time.

Functional programming language use a nice technique to force to handle the failure case:

Here in laravel we can return a Nullable object from out method.

/**
* @return Nullable        <---- we now have only one consistent type.not two.
*/
public function find ($id) {
     $user = TwitterApi::search($id);

     if (!$user) {
         return new Nullable();
     }
     $user = new User($user);   

     return new Nullable($user);
}

Now there is no way for the caller to forget handling the failure of search. Why ? because they can only gain access to the User object if they specify the code to run if was null was returned.

// immediately send a response
$userObj = $userRepo->find($id)->getOrSend(function () {
  return redirect()->route('page_not_found');
});

// Call a static method.
$userObj = $twitterApi->find($id)->getOrSend([Response::class, 'pageNotFound']);

// or a get default value
$userObj = $twitterApi->find($id)->getOr(new User());

// or execute a default action
$userObj = $twitterApi->find($id)->getOrDo(function() {
    // do some stuff.
    return new User();
});

I have written the code as a proof of concept package which is very small indeed.

https://github.com/imanghafoori1/laravel-nullable

mfn commented 5 years ago

Nice indeed.

But I'm 👎 including something like this in Laravel at this point.

Backwards compat aside, this doesn't make sense to me unless there is support for generics on some level.

imanghafoori1 commented 5 years ago

Indeed I meant for laravel 6.0 version.

support for generics

I do not get what you exact mean about generics, but any new idea from community makes me happy.

rimace commented 5 years ago

Interesting. In my company we have items which can have a group (for example). In the last years there were here and there a few cases were I forgot to check if this relation is null. But as 99,9 % of the items had that relation, I noticed the error month or years after going live. So I wonder if this could have prevented this. I guess to prevent my mistakes even the relation must be declared as nullable, right? For instance $item->group must return a Nullable object. That means in the view I always need to declare a variable like $itemGroup = $item->group->getOr(new Group()). I like your thoughts, but it seems the views would get ugly.

martinbean commented 5 years ago

Yeah, this seems to introduce a lot of code.

If a method returns a “nullable” and I forget to call any method on it, then my application’s going to break, just as it would if I try to call a method on null. So I’m not sure what the gain here is?

imanghafoori1 commented 5 years ago

@martinbean The point is that your application either doesn't work at all, or works properly. You won't get sneaky run-time errors.

If a method returns a “nullable” and I forget to call any method on it,

That is a syntactic error that your IDE highlights it for you. You get auto-completion you nullable methods as well.

imanghafoori1 commented 5 years ago

An other advantage is that, if you use nullable and you forget to write a test that simulates the situations where null values are returned, phpunit code coverage highlights the closure you have passed to the ->getOrDo() (or similar methods) as none-covered, indicating that there is a missing test.

but if you return the object directly, you can get 100% code coverage without having a test covering nully situations, hence hidden errors may still lurk you at 100% coverage.

martinbean commented 5 years ago

Honesty this is not something I have invented. It is a widely used concept in function programming languages world.

@imanghafoori1 No one accused you of making this up or inventing anything?

autaut03 commented 5 years ago

No. Such things could replace null (like Optional in java), but they should definitely NOT replace exceptions and they are not meant to.

Exceptions with proper exception handling (which, thankfully, is more or less done good in Laravel out of the box) are the way to go. There's no need to be checking for null every time you fetch something, nor is there a need to write a custom exception every single time you need to stop processing if your model doesn't exist.

Say you have a controller, which calls a service, which calls a service, which calls another service, which fetches a model by id. A pretty realistic situation in big projects. Currently, you would just do findOrFail and that's it. If model's not there, an exception is thrown and passed down the trace, right until other service/controller catches it. If it's not catched in those, there's always an exception handler. Transaction's not been commited, everyone's happy. Except the user, of course, but your way of doing it doesn't change anything for them.

What would I do with your approach? Start rewriting the whole project to some implementation-dependent composer-package class to achieve what? To have thousands of checks to not throw a simple exception? No.

You won't get sneaky run-time errors. - I'm sorry, but it's not like this problem is solvable just by introducing a "nullable". This has to be on a language level, which is a direct road to c#/kotlin/whatever-strictly-typed-language-with-nullable-types-you-like. I'm fine with that, but definitely not the way you implemented and definitely not on framework level.

imanghafoori1 commented 5 years ago

The nullable is not a magic thing to replace everything with, of course. When exceptions are fine, ok they are fine.

Laravel also has 'optional' function to deal with null at framework level.

Nullable are useful when someone writes a package and others download it en mass. The package maintainer is better to return nallables to people than a raw helish null. If he is not going to throw a framework handled exception.

martinbean commented 5 years ago

Nullable are useful when someone writes a package and others download it en mass.

@imanghafoori1 So why are you trying to add this to a framework if it’s actually package authors that—in your view—should be employing it?

PHP (thankfully) isn’t a language that has “optional” types built-in. I do a fair bit with Swift and its Optional type system is a pain. Constantly having to check if things are nil, unwrapping, etc. I can’t think of anything worse than if I had to tack a getOr<DoSomething> call on the end of every method call in a framework/package/application.

autaut03 commented 5 years ago

The package maintainer is better to return nallables to people than a raw helish null. If he is not going to throw a framework handled exception.

Package maintainer is better to not be dependent on user-level implementation of nullables. Package maintainer is better to throw actual exceptions rather than nulls. Most of the time you want to know what kind of problem has occurred alongside details of the problem, rather than a stupid null that came who-knows-from-where and what's the reason of that null response. Exceptions are ideal for that.

imanghafoori1 commented 5 years ago

@autaut03 That nullable package is just proof of concept and is not meant to be used in production as is, I like to know what issues other smarter people can see about that, which I missed.

Framework level exceptions are not always a possible choice and custom exceptions are subject to be forgotten like nulls are.

imanghafoori1 commented 5 years ago

@martinbean At language level it becomes sometimes painful because everything is optional. but at framework level we can can choose to return nullables when we are really worried about handling null cases.

Anyway it is a suggestion to be discussed.

autaut03 commented 5 years ago

That nullable package

I've never said anything about your specific package. Any implementation of this concept is just plainly a really bad practice, at least because it's someone's package which is nowhere near language level things.

Framework level exceptions are not always a possible choice

Framework exceptions are for the framework and, sometimes, for the application that uses that framework. That's it. Anything else should never depend on them.

custom exceptions are subject to be forgotten like nulls are.

Lol. If you declare such things, prove them. Besides, I have some questions regarding this:

Answer all four. With real PHP code examples. If you know how to solve all these without custom exceptions and also being easier to use (to be honest, besides syntax sugar for try-catch it can't possibly get much easier) - submit an RFC to PHP. Otherwise, don't say such things.

imanghafoori1 commented 5 years ago

Nice, with nullables it is possible to send error message in case of null happens. I will implement that for my package. For example : "Model not found." is needed to pass along.

These links may provide good answers to your questions : https://doc.rust-lang.org/stable/rust-by-example/std/option.html https://doc.rust-lang.org/stable/rust-by-example/error/result.html

Using these ideas a modern language like rust has managed to eliminate the need of try/catch/exceptions. (although we do not want to do that here.) and they did not design a bed language. 2019 : https://insights.stackoverflow.com/survey/2019#technology-_-most-loved-dreaded-and-wanted-languages 2018 : https://insights.stackoverflow.com/survey/2018#technology-most-loved-dreaded-and-wanted-languages 2017 : https://insights.stackoverflow.com/survey/2017#most-loved-dreaded-and-wanted 2016 : https://insights.stackoverflow.com/survey/2016#technology-most-loved-dreaded-and-wanted

This course also describes why thrown exception are subject to be forgotten to be caught and handled properly by users. https://www.pluralsight.com/courses/advanced-defensive-programming-techniques

All and all I think for the same reason we already have optional() helper function we can have a very simple nullable() helper function.

optional silently suppresses the null reference errors. nullable will force to provide an alternative way to continue or a proper default value. All I want to say is just that, I do not see anything nasty with nullables.

autaut03 commented 5 years ago

send error message in case of null happens

One more problem. Your controllers shouldn't contain any business logic, and your business logic shouldn't contain any references to HTTP. So your service, which handles case of something being null, must NOT return an HTTP response, ever.

These links may provide good answers to your questions :

They don't. They provide a single example with a division error, seriously? Show me a real-world complex application in Rust with real business-related errors.

This course also describes why thrown exception are subject to be forgotten to be caught and handled properly by users.

I'm sorry, but I'm not going to buy a subscription/trial for the course, nor will I waste hours on some course because you can't provide clear answers to my questions regarding your statement.

we already have optional() helper function

Did.. you just compare a helper that's used once a day at max to a complete rewrite to all Laravel projects?

All I want to say is just that, I do not see anything nasty with nullables.

Maybe there isn't, but after trying out real nullable types in Kotlin and Optional in Java, I'll say that I'd choose real nullable types any time of the day. And guess what? They're already part of the language! There's no need for other kind of solution.

Both exceptions and nullable types are easy to work with, simple to understand, don't need any additional wrappers or support for generics, don't force developers to write tons of boilerplate and actually allow the developer/IDE/compiler to decide how to deal with nulls/errors, not third-party magical object.

imanghafoori1 commented 5 years ago

No one is force to rewrite, it's just a helper function in the framework and people can choose to use it when ever they feel comfortable.

martinbean commented 5 years ago

@imanghafoori1 You seem to keep proposing ideas and concepts from other languages. Have you just considered using another language instead of trying to morph a framework written in a language that doesn’t have first-party support for the concepts you’re extolling?

As mentioned, I’ve used optionals in Swift (a “new” language) and they’re a pain to code around: constantly passing, checking, unwrapping. I’d rather just have a method return something or throw an exception. I don’t want to be sticking getOrWhatever() calls on the end of every method call because it’s giving me a pseudo-nullable value back.

You also seem to be unclear about your intention. You’ve opened this issue on the Laravel framework ideas thread, then when challenged said it should be package creators that should implement this concept. Like, what is it you’re proposing? Give concrete examples. @autaut03 has tried but you just keep sharing academic material rather than giving a concise, concrete example.

You’re not going to get much traction if you just keep proposing ideas because “language X has them”. There needs to be an unequivocal, tangible benefit. I’m pretty sure I’ve said this to you before. Because otherwise, Taylor and the Laravel team are not going to waste their time completely re-factoring Laravel based on something someone read about another language.