Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Change return type of Model::findFirst or throw exception #14044

Closed
Jurigag opened this issue May 7, 2019 · 11 comments

Comments

Projects
7 participants
@Jurigag
Copy link
Member

commented May 7, 2019

Simple as that. It feels weird that Phalcon\Mvc\Model::findFirst returns object or false, why false exactly, i never undestood this really. Null makes more sense and also it works with nullable types in PHP 7 and above.

I propose to add it to v4 while we still can.

@Jurigag Jurigag added this to the 4.0.0 milestone May 7, 2019

@virgofx

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Agree. I believe the return false is somewhat reminiscent of legacy/older PHP functions that often return false. It's a bad practice.

I would be in favor of actually moving everything to use exceptions and then have proper return types. I think we should use proper returns everywhere. Thus the return type would be @return static with an annotation for @throws \Phalcon\Model\NotFoundException or something. We should use exceptions everywhere and drop the global/orm option that defaulted this to false in 4.0

Static analysis would improve so much further as well doing this.

@Jurigag

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

I guess that's even better idea. I will change title.

The only problem with this is that changing it will require a lot of changes if someone already checks if findFirst returns false with === and return null themself. Maybe just make it optional using globals to keep older behavior and remove it later.

I guess we can as well ignore it since it will be major version.

Also what about Model::find? Some people would argue if we have empty array result we should also throw exception and i could agree with it, what's your opinion?

What is good about exception that in most simple cases we could just catch it and return 404.

@Jurigag Jurigag changed the title [RFC]Return null instead of false on Model::findFirst [RFC]Change return type of Model::findFirst or throw exception May 7, 2019

@virgofx

This comment has been minimized.

Copy link
Member

commented May 7, 2019

I would argue that find() should always return a ResultSet instance/interface 100% of the time such that static analysis can work. One of the interfaces from Complex/Simple. Even if it's empty.

And I would say that we really should drop the ORM option with respect to findFirst(), save() , (and if there are any other areas?) ... those should always throw proper exception. By removing the global option, it makes it more concise for our examples, documentation, code, references, etc.

Best practice is the developer should handle their exceptions as necessary. Catch and return custom JSON, arrays, custom view logic, or simply allow the error to bubble and be handled by default exception/error logic. It will automatically lead to better programming and cases of hidden bugs where people attempt to operate on a returned result of null|false (even if it's less common).

@virgofx virgofx changed the title [RFC]Change return type of Model::findFirst or throw exception [RFC] Change return type of Model::findFirst or throw exception May 7, 2019

@virgofx virgofx added Feature - NFR and removed Feature - NFR labels May 7, 2019

@sergeyklay

This comment has been minimized.

Copy link
Member

commented May 8, 2019

/cc @niden

@niden niden added this to To do in 4.0 Release via automation May 10, 2019

@crlikcngroup

This comment has been minimized.

Copy link

commented May 10, 2019

I agree that false value can be bad practice.

But in our case it was quite useful. In caches for example. When attribute is null, you can tell entity was not queried. When false, you can discriminate between already queried but does not exist.
But a lot of programmers has a problem with this. So if it was handled via exceptions for example, cool.

@scrnjakovic

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I disagree on couple of points:

  1. Why should Model::findFirst throw an exception if nothing is found, and Model::find should return ResultSet?

  2. Having exceptions being thrown at you for doing a simple query is wrong and costly. I don't think exceptions are meant to be used this way. It implies that record is always expected to exist in the database, which is really wrong. Exceptions should be used to signal serious errors from which a method is unable to recover. Fetching something if it exists, only to find out it doesn't, isn't an exceptional state.

LINQ has .first method that throws an exception if it yields empty resultset, but this method was designed to assume at least one record exists. It also offers .firstOrDefault which yields null if nothing is found.

Model::findFirst and Model::find are not designed with this assumption, are they?

@Xplouder

This comment has been minimized.

Copy link

commented May 14, 2019

Forgetting the exceptions, I would be happy with just null for findFirst and getRelated.

@Jurigag

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Well i agree that that's it not necessary that we throw any exception, this is just thing to consider. We can always add method findFirstOrDefault, or something like this, or even make it global configuration in Model::setup()

Exceptions aren't serious signals from which you can recover, those are fatal errors, they are just information that something went wrong - in that case - you don't have any result, you can handle it easly. But having exception you can easly handle it on any level you want - in code where you exactly calls findFirst, or in controller action if you do for example many findFirst but if any of the records doesn't exist - you want to return 404 or even globally if you just want always to return something etc.

For me null is the option we should consider at least - to make possible things like :?AnyClass, nice and easy.

Though it won't be so clean because we can always have Row returned, but i guess this should be covered by developer, or we should implement partial models, but it's other feature to consider.

@niden

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Making this change will of course break backwards compatibility. Since this is not a rewrite or changing of a pattern (say Active Record vs. Data Mapper), I think we should try and minimize the pain that a developer will have to go to refactor their code. Clearly the null approach is the path of least resistance. Utilizing Exceptions will definitely need a lot more work if not refactoring existing classes that could very well have exceptions themselves.

If we move to Exceptions, we will also have to consider them when using save(), update() etc, which will increase the complexity and scope of this task.

I am leaning towards:

  • findFirst
    • Resultset if found
    • null if not found
  • find
    • Resultset collection if found
    • Empty Resultset collection if not found

@niden niden referenced this issue May 17, 2019

Merged

T14044 model findfirst null #14089

4 of 4 tasks complete

@niden niden removed this from the 4.0.0 milestone May 17, 2019

@niden niden moved this from To do to In progress in 4.0 Release May 17, 2019

@sergeyklay

This comment has been minimized.

Copy link
Member

commented May 17, 2019

There is also findFirstBy<property-name>() eg. findFirstByEmail().

@niden

This comment has been minimized.

Copy link
Member

commented May 18, 2019

Resolved in #14089

@niden niden closed this May 18, 2019

4.0 Release automation moved this from In progress to Done May 18, 2019

@niden niden added the 4.0 label Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.