Can't fully reset query objects.

[alanbem]

No description provided.

[alanbem]

I have tried to test as much possible but could miss something. I think in-depth review is neccessary.

[staabm]

could you separate the phpdoc changes into a separate commit? would make it easier to review the actual changes for the request (best would be a separate PR but I would be ok with it here though)

[staabm]

better split this up into separate test methods?

[staabm]

I am not convinced yet if those tesets have a benefit, because when we introduce new properties in the future those would not be covered by the tests and probability to miss extending this tests would be high, I think...

@willdurand @marcj @havvg @jaugustin whats your opinion on this?

[havvg]

I'm +1 on separating this test case.

Your probability on missing something is already 100%.
In Criteria::clear() there are already two properties (ifLvlCount, wasTrue) "reset" while they don't exist (anymore).

[staabm]

@havvg so you are ok with adding those tests?

[havvg]

Hm.. just thought about it, adding a new property will actually not be covered, as long as it is not altered by clear.

clear as it is also very plain to read. I would suggest to reduce the testcase to call clear on a ModelCriteria and check it is "equal" to a new one, which is the purpose of clear, this will verify it's at least reset correctly and clear does not set up a different state.

[alanbem]

Really? We've got now bunch of tests that covers most criteria object mutations - in context of clear method - and you want to throw them away? :(
At least we will be warned in the future, if one of currently existing features will break clear method - so I would let those tests stay.

[havvg]

This case is about testing the method clear not those mutators. The mutators (should) have their separate cases (e.g. ModelCriteriaTest::testFilterBy). You are mixing the SUT here. It also tests the same thing multiple times, like adding a join in different ways (which is covered already).

As I said, I'm not against having such a test, but it has to be reasonable, like CriteriaTest::testClear.

[staabm]

hmm thinking the other way arround. In case we add a new property which would be forgotten in clear(), this would be considered a bug. this bug would be proved by a failing test-case and therefore fixed. So this would lead to something we have here in this PR.

Therefore I think we should have one test, which creates a criteria, changes more or less every property of it, assert it is not equal to a plain criteria anymore, but will be equals again after calling clear(). One separate Test per Property seems not worth to blow up the wohle suite...

[havvg]

Sounds good to me, then you won't need to have every possible way to change those properties, but one changed Criteria to be cleared. It would be a huge arrange step, but very small act and assert steps - which makes it readable again and is reasonable again.

[alanbem]

It also tests the same thing multiple times, like adding a join in different ways (which is covered already).

I am not familiar with all Propel internals, so feel free to remove any redundancy later on.

Therefore I think we should have one test, which creates a criteria, changes more or less every property of it, assert it is not equal to a plain criteria anymore, but will be equals again after calling clear()

This was my initial idea, but I assumed it could make test unreadable.
Nevertheless I like it - it is sane and pragmatic. Also reducing current tests acording to your proposal is quite easy.

[staabm]

when the default will no longer be obvious from the constructor signature, I would love to see the phpdoc generated in a way which mentions the value to which null will fallback. see https://github.com/alanbem/Propel/blob/27ea1df98bbc9e3f66f24047ba943a7fd1cb7aea/generator/lib/builder/om/QueryBuilder.php#L266

[havvg]

What's the reason for this change?

You drop the default from the signature in favor of moving it into the constructor logic, why?

[alanbem]

Long story short: this is required to allow preserve initial alias after clear() for queries created by `ModelCriteria::create()

see https://github.com/alanbem/Propel/blob/cd22e0bbc2a9ec14c0fa3d72abe6020074ac1997/test/testsuite/runtime/query/ModelCriteriaTest.php#L2593

At the same time this change is backward compatible with possible custom query objects like

class CustomAuthorQuery extends ModelCriteria
{
    public function __construct($dbName = 'bookstore', $modelName = 'NOT_DEFAULT_MODEL_NAME', $modelAlias = null)
    {
        parent::__construct($dbName, $modelName, $modelAlias);
    }
}

[havvg]

Wait what? I can't follow you, how do those two code snippets differ? What's the reason to use the latter instead of the first one?

public function __construct($dbName = 'foo')
{ 
    // .. 
}

public function __construct($dbName = null)
{
    if (null === $dbName) {
        $dbName = 'foo';
    }

    // ..
}

[alanbem]

Okay, again :)

After my first ModelCriteria::clear() fixes this is how clear() worked with $modelAlias

$c = new ModelCriteria('bookstore', 'Book', 'alias1');
$c->clear();
echo $c->getModelAlias(); // alias1, correct!

but

$c = BookQuery::create('alias1');
$c->clear();
echo $c->getModelAlias(); // null, something is wrong with create()!

Unfortunately initial alias of query created by factory was not preserved after clear(), so I've looked at generated base query. It was something like:

abstract class BaseBookQuery extends ModelCriteria
{
    public function __construct($dbName = 'bookstore', $modelName = 'Book', $modelAlias = null)
    {
        parent::__construct($dbName, $modelName, $modelAlias);
    }

    public static function create($modelAlias = null, $criteria = null)
    {
        if ($criteria instanceof BookQuery) {
            return $criteria;
        }
        $query = new BookQuery();
        if (null !== $modelAlias) {
            $query->setModelAlias($modelAlias);
        }
        if ($criteria instanceof Criteria) {
            $query->mergeWith($criteria);
        }

        return $query;
    }
}

The problem was

        if (null !== $modelAlias) {
            $query->setModelAlias($modelAlias);
        }

because $modelAlias was set via setter, query couldn't know that it was initial alias and clear() would turn it into null. So I've modified a query generator a bit:

    public static function create($modelAlias = null, $criteria = null)
    {
        if ($criteria instanceof BookQuery) {
            return $criteria;
        }
        $query = new BookQuery('bookstore', 'Book', $modelAlias);
//        $query = new BookQuery();
//        if (null !== $modelAlias) {
//            $query->setModelAlias($modelAlias);
//        }

        if ($criteria instanceof Criteria) {
            $query->mergeWith($criteria);
        }

        return $query;
    }

the code below started to work

$c = BookQuery::create('alias1');
$c->clear();
echo $c->getModelAlias(); // 'alias1', correct!

But hardcoding $dbName and $modelName

        $query = new BookQuery('bookstore', 'Book', $modelAlias);

had one huge drawback, if someone customized their BookQuery like that

class BookQuery extends BaseBookQuery
{
    // note modified $modelName
    public function __construct($dbName = 'bookstore', $modelName = 'ExtendedBook', $modelAlias = null)
    {
        parent::__construct($dbName, $modelName, $modelAlias);
    }
}

BookQuery::create() would stop to work as before my changes - my code broke BC. So I've refactored generator one more time

// this is current code in repo
abstract class BaseBookQuery extends ModelCriteria
{
    public function __construct($dbName = null, $modelName = null, $modelAlias = null)
//    public function __construct($dbName = 'bookstore', $modelName = 'Book', $modelAlias = null)
    {
        if (null === $dbName) {
            $dbName = 'bookstore';
        }
        if (null === $modelName) {
            $modelName = 'Book';
        }
        parent::__construct($dbName, $modelName, $modelAlias);
    }

    public static function create($modelAlias = null, $criteria = null)
    {
        if ($criteria instanceof BookQuery) {
            return $criteria;
        }
        $query = new BookQuery(null, null, $modelAlias);
//        $query = new BookQuery('bookstore', 'Book', $modelAlias);

        if ($criteria instanceof Criteria) {
            $query->mergeWith($criteria);
        }

        return $query;
    }
}

et voila - works like a charm and is as backward compatible as it can get.

[havvg]

Thank you very much for the outline!
I missed the difference when actually calling the function; would be great to see new BookQuery(default, default, $modelAlias) become available in PHP.

[staabm]

Great write-up!

[staabm]

@alanbem you got it right. despite separating the tests this LGTM / looks good to me

[willdurand]

Feel free to split the test in a separate PR, it's ok to me.

[willdurand]

Thanks!