Throw exception for conditional flow methods without underscore prefix

[oojacoboo]

Summary

• PropelConditionalProxy::__call() now detects calls to conditional flow methods (if, elseif, else, endif) without the required underscore prefix and throws a BadMethodCallException
• All other method calls are still silently skipped as before, preserving compatibility with virtual methods (filterByX, orderByX, etc.) handled by Criteria::__call()

Problem

The previous __call() implementation silently returned $this for all method calls. This meant that calling ->endif() (instead of ->_endif()) would be silently swallowed by the proxy, along with all subsequent chained calls (filterBy*, find(), etc.), producing silently incorrect query results.

This is an extremely difficult bug to diagnose — the query appears to execute normally but returns wrong data because the conditional block is never properly closed.

Solution

Added an explicit check for the four conditional flow method names (if, elseif, else, endif) called without their underscore prefix. These are the only methods where calling without the prefix is always a mistake (the real methods are _if, _elseif, _else, _endif).

All other arbitrary method names continue to be silently skipped, since Criteria and ModelCriteria use __call for virtual/magic methods like filterByCompanyId, orderByName, etc.

Test plan

• Added test: valid criteria methods are still silently skipped when condition is false
• Added test: arbitrary method names (virtual methods) are silently skipped
• Added data provider test: all four conditional flow methods (if, elseif, else, endif) without underscore throw BadMethodCallException
• Existing CriteriaFluidConditionTest::testIf passes (uses ->foo() as arbitrary method)

[kasparsatke]

Wow! Awesome you have spotted this! Strange though that this has gone unnoticed for so long. Do you think it also makes sense to add ->_or()/ ->_and()? Their impact when mistyped is surely not so heavy but still would silently produce unintended queries. Or are those not affected and already throw when mistyped?

[oojacoboo]

@kasparsatke yea, not sure. I know I've run into this issue before as well. It's always been strange to me that underscore methods were chosen for these, instead of just making them reserved. Another option would be to add non-underscore aliases for them. But either way, this will prevent these mistakes going forward.

I added and and or too - good call.

[mringler]

Very nice! Though I am not sure if this line of user-friendliness and forward thinking actually fits with the inherited project guidelines. Wouldn't we rob later users of that hour of fun you had figuring out why the damn query is not working? Are we sure users don't actually like it when they feel tricked?

But seriously, I think there is also an issue with typing here. Shouldn't the conditional methods be typed to return static (even though they don`t)?

Currently, it goes:

    FooQuery::create()
      ->filterByFizz()          // FooQuery
      ->_if(...)                // Criteria|ConditionalProxy
          ->filterByBuzz()      // unknown method ("passed to _call of Criteria")
      ->_else()                 // don't know what this is even called on
          ->filterByFizzBuzz()  // hope you know
      ->end()                   // what you are doing
      ->filterByFoo()           // good luck and have fun typing it out!

Should we use the occasion and think about improving this too?

[mringler]

I'm constantly replacing these sprintf calls in the code, if I ran into this, I'd almost mechanically change it to:

$className = get_class($this->criteria);
throw new BadMethodCallException("Call to undefined method {$classname}::{$name}(). Did you mean '_{$name}' (with underscore prefix)?"

Easier to see what goes where, fewer lines, no nested call.
Is there a case for sprintf that I am not seeing?

[kasparsatke]

The only use case where I ever use sprintf() is in combination with gettext() when a translatable string has parameters to be replaced. Other than that I never use sprintf().

So if you are not getting an itch right now to internationalize our exception messages, just proceed changing ...

[oojacoboo]

Yea, no reason really. I tend to use both. You can't use method chained calls with string interpolation, which often gets me when trying to use it.

Updated it - how's that? You can see by my commits how string interpolation in PHP can be frustrating to deal with. It's unfortunate. I really wish we had Javascripts template strings.

[mringler]

I think in interpolation, you can do method calls on objects, but nothing static and no global functions. For the holy cow of readability, I do prefer variables anyway. Something like "The car '{$car->getName()}' is {$car->getColor()}" is ok, but just not as good as "The car '$carName' is $carColor".

No need for newlines around the message IMO, it is too long anyway, and it creates the appearance that something unexpected is going on there. Admittedly this gets into preference/pedant territory. It's fine! But it's again something that I might change almost mechanically when passing by.

Oh, by the way, talking about testing via commits, that's alright of course, but in case you missed the announcement, there are docker container configurations at perplorm/perpl-test-docker that should make it ease to run the tests locally. Works great for me, far less hassle.

[oojacoboo]

@mringler nice, I'll test those those Docker containers next time.

On the variables vs method calls. Yes, sometimes a variable name can add clarity. However, it also takes up memory, so it's something to consider.

Chained method calls don't work in string interpolation, $car->getBrand()->getName()

[oojacoboo]

Very nice! Though I am not sure if this line of user-friendliness and forward thinking actually fits with the inherited project guidelines. Wouldn't we rob later users of that hour of fun you had figuring out why the damn query is not working? Are we sure users don't actually like it when they feel tricked?

But seriously, I think there is also an issue with typing here. Shouldn't the conditional methods be typed to return static (even though they don`t)?

Currently, it goes:

    FooQuery::create()
      ->filterByFizz()          // FooQuery
      ->_if(...)                // Criteria|ConditionalProxy
          ->filterByBuzz()      // unknown method ("passed to _call of Criteria")
      ->_else()                 // don't know what this is even called on
          ->filterByFizzBuzz()  // hope you know
      ->end()                   // what you are doing
      ->filterByFoo()           // good luck and have fun typing it out!

Should we use the occasion and think about improving this too?

This should probably be addressed as well. But I think it should be another PR.

[mringler]

Alright, let's tackle the types in another PR