gabordemooij / redbean

ORM layer that creates models, config and database on the fly
https://www.redbeanphp.com
2.31k stars 280 forks source link

confusing default behavior: calling non-existent method on bean gives no error #872

Closed ipsod closed 2 years ago

ipsod commented 3 years ago

This code gives no error:

R::nuke();
$f = R::dispense('f');
$r = $f->hey_there();

It just sets $r to NULL. This is error-prone and makes debugging difficult. Any other class would give an error.

I had to look hard to find mention of it in the documentation. It's hidden under the heading "Custom FUSED methods". And, though I found mention of it, it's not clear what to do next to fix it:

If you call a method on a bean that does not exist in the bean and also not in the model the call will be ignored. You change this behaviour by selecting a different FUSE error handling mechanism using the setErrorHandlingFUSE() method, see API.

So, I'm digging through that now, but... why is this your preferred behavior? The downside seems obvious. Is there some benefit?

ipsod commented 3 years ago

I've tried to make it throw an error. I've tried

R::setErrorHandlingFUSE(RedBeanPHP\OODBBean::C_ERR_FATAL);

and

R::setErrorHandlingFUSE(RedBeanPHP\OODBBean::C_ERR_EXCEPTION);

but neither seemed to have any effect.

Are you really such good coders that you don't accidentally call non-existent functions even though your ORM breaks auto-complete? Why break error-reporting, as well? It makes broken code appear to work, and leaves you wondering what's going wrong where.

Like, why are my products not showing up? (...30 minutes later) Oh, because I called $category->ownProductList(), and not $category->ownProductList. An error would have been nice.

ipsod commented 3 years ago

I see now that these settings do work, when I'm in the browser. They don't work in PHPUnit tests, though. Is there a way to make them?

final class LearnRedBeanTest extends TestCase
{
    public function test_is_redbean_insane() {
        R::setErrorHandlingFUSE(RedBeanPHP\OODBBean::C_ERR_EXCEPTION);
        R::nuke();
        $f = R::dispense('f');
        $r = $f->hey_there();
        $this->assertNotEquals($r, null);
    }
}

result:

There was 1 failure:

1) RedBeanTest::test_is_redbean_insane
Failed asserting that null is not equal to null.
ipsod commented 3 years ago

I see now that these settings do work, when I'm in the browser. They don't work in PHPUnit tests, though. Is there a way to make them?

To be clear, on any other class, calling a non-existent function does show errors on the terminal output from a phpunit test, but this example tests as though I have not called R::setErrorHandlingFUSE(RedBeanPHP\OODBBean::C_ERR_EXCEPTION);.

Lynesth commented 3 years ago

I understand the frustration. Before I get into details as to why it's currently like that, here is the smallest changes we'd need to make to the __call method of the OODBBean so that it works how you want it to while still being backward compatible (mostly):

--- a/RedBeanPHP/OODBBean.php
+++ b/RedBeanPHP/OODBBean.php
@@ -1380,16 +1380,19 @@ class OODBBean implements \IteratorAggregate,\ArrayAccess,\Countable,Jsonable
         */
        public function __call( $method, $args )
        {
-               if ( empty( $this->__info['model'] ) ) {
-                       return NULL;
-               }
-
                $overrideDontFail = FALSE;
                if ( strpos( $method, '@' ) === 0 ) {
                        $method = substr( $method, 1 );
                        $overrideDontFail = TRUE;
                }

+               if ( empty( $this->__info['model'] ) ) {
+                       if ( $overridDontFail || in_array( $method, array( 'box', 'unbox', 'update', 'open', 'delete', 'after_delete', 'after_update', 'dispense' ), TRUE ) ) {
+                               return NULL;
+                       }
+                       throw new \BadMethodCallException("Method $method doesn't exist and bean has no model");
+               }
+
                if ( !is_callable( array( $this->__info['model'], $method ) ) ) {

                        if ( self::$errorHandlingFUSE === FALSE || $overrideDontFail ) {

Now a few informations to understand why you didn't get it to work and why it got where it is now.

You are not getting any error in your tests because you don't have a model for your bean. As you can see in the code just above, in the current version, if you don't have a model, it simply returns NULL.

Each time something happens to a bean, some events are automatically called, like update, dispense, etc. This is done whether or not the bean as a model. Also, beans are supposed to be boxable and unboxable (accessing the model from the bean and vice versa) without having to first check if we're calling it on a bean or on the model, for ease of use iirc.

For those reasons, the __call method was implemented as it currently is. Modifying that now might break a lot of things for some people. Also we wouldn't do it the way I wrote it above (which really is just patchy way to "fix" it) and so would require more work. From what I remember, it is the first time this has been brought up, although I haven't been here since the beginning so I wouldn't know.

I think that making it so that it fires an exception when the method doesn't exist would be a good idea but that means more than a couple lines of code and a probably difficult backward compatibility.

ipsod commented 3 years ago

Thank you for your detailed response!

Ah, that explains why I'm having this problem in my tests but not in my app. I guess I started out getting no errors in my app, then turned error reporting on after I'd already stripped the test down to the bare minimum, leaving this test broken (since I'd stripped out its models).

This might be a good reason to break backward compatibility, after a few deprecation warnings or something? At least a really attention-grabbing warning in the docs? I ran into this problem multiple times, not just the once, and if I wasn't so far into my project I'd have given up and gone back to Django.

Yes, being able to box and unbox without caring what type is good for ease of use - I actually discovered that feature because I went looking for it because it was tedious trying to verify type before calling.

gabordemooij commented 3 years ago

Hi @ipsod,

Too bad you experience some problems while using RedBeanPHP Fuse. Thanks @Lynesth for your detailed explanation. There is not a lot I can add to this explanation. This mechanism has kind of evolved to where it currently is. To be honest (but I am the original author so I am biased) I never have issues with this. Sure, sometimes I forget to hook up the models and things break, but that's almost always only in the beginning of a project. I use RedBeanPHP for all my projects btw. It might also have to do with the way I use RedBeanPHP. If you like you can send me a snippet of your code and we can discuss it. Of course, coding style is a matter of opinion, but RedBeanPHP has been designed with a certain style in mind, so it could be beneficial to see how you use it, and how RedBeanPHP can fit in. Personally, I like to keep things simple. I personally use RedBeanPHP together with my own little micro framework called 'Straight' https://gabordemooij.com/straight/ - but that is certainly not for everyone. Anyway, wish you good luck and thanks for discussing this with us.

cheers, Gabor

ipsod commented 3 years ago

Hi @gabordemooij ,

I will try "Straight". I looked over the code, and I like it - not that I know much, but it was simple enough to be immediately comprehensible, obviously fast, and clearly better than what I was doing.

Thank you for your offer to discuss my code - I hope to take you up on it soon!

Is there a preferred place to ask questions? Also, is there a place to find example projects that use best-practices? Or, docs covering common tasks, like making/validating a form and showing errors? I'm finding that many things take me much longer than they would have Django, but it's mostly because I'm approaching them with no docs and no recent experience with PHP, so I'm having to create/discover a solution from scratch instead of just implementing one from memory or docs.