makeWithModelListenersAndBehaviors should be a transient state change, not permanent

[Zuluru]

makeWithModelListenersAndBehaviors as currently implemented sets the applyListenersAndBehaviors property of the factory, and leaves it set. I have two concerns with this: one philosophical and one practical.

The philosophical issue is that the function name does not reflect that it will be permanently changing the state. A name like enableModelListenersAndBehaviorsAndMake would be clearer about this. But secondly, modern design principles prefer that functions not have two effects; if you need to use "and" to describe what a function does, it should generally be split into two functions.

The practical issue is that if I have testA and testB, where testA calls makeWithModelListenersAndBehaviors but testB uses only make, then the behaviour of testB will depend on whether testA was run before it or not, because it's not just the concrete factory object that's affected by this, but a static property that make references.

[dreamingmind]

@Zuluru, I was under the impression that the overall process of testing in phpUnit would clear the slate between tests so I checked this with one of my tests that use this call to prove your concerns were unfounded.

To my surprise, the flag to enable listeners and behaviors did persist in later tests methods once it was enabled.

That was a bit of a surprise!

[dreamingmind]

There are some other save and marshaling options that can be set. I wonder if those also persist.

Is this something that the FixtureFactory listener could simply reset to a default state at the same time the previous test data is cleared?

[Zuluru]

My experience has been that anything global remains from one test to the next. Static properties are an example. Also defined constants. phpunit does not start a new PHP instance for each test, it just resets all the things that it knows about. Cake's standard setUp and tearDown functions reset some more things that they know about (configuration, routing, table locator). My own setUp and tearDown functions do a little bit more...

[pabloelcolombiano]

Thanks for the feedback.

I agree that the handling of listeners and behaviors may be treated separatly.

I will remove the heavy makeWithModelListenersAndBehaviors static method, and instead propose two methods,
enableListeners and enableBehaviors so they can be set up on the fly. These should not be set staticly, so every Factory start with both behavior and listeners set to false. In addition, it is possible this way to set them to true in the setDefaultTemplate method if required.

[Zuluru]

I don't mind makeWithModelListenersAndBehaviors being available as a convenience method that sets it temporarily and cleans up after itself, but the other methods are good additions, and should be sufficient on their own if you think that's the better interface.

[pabloelcolombiano]

Behaviors were disabled mostly because they generally work with Model Events, and our feeling was that Fixture Factories should insert data as fast and as blind as possible in the test DB. But this is actualy not necessary.

Facit in the new versions v0.2.25 and v0.1.20:

1. I have renamed the method makeWithModelListenersAndBehaviors into makeWithModelEvents. Sorry for those who already used the previous method. I do not wish to deprecated it, let's be direct as long as the package is still young.

2. make will insert data without listening to model events at all, including events called within behaviors (same as before).

3. makeWithModelEvents will insert data while listening to all model events, including those in the Behaviors.

[Zuluru]

I'm now finding that tests that are testing lifecycle events are failing. Need to be able to:

1. Create a record through the factory without those events
2. Patch the entity and save it
3. Have events like beforeSave and afterSave run in the table
4. Test then asserts that the events happened and expected effects have been applied

Not sure if I should report this here or as a new issue...

[pabloelcolombiano]

What happens if you consider an ArticlesTable and do:

1. $article = ArticleFactory::make()->getEntity(); // creates an entity, without persisting it. No Model events are triggered.
2. $ArticlesTable->save($article); // Save the article the CakePHP regular way.
3. Events Model including beforeSave and afterSave are triggered by the action in point 2.
4. Assert that you obtained what you expected.

Does this work?

[Zuluru]

That would probably work for some scenarios, but many check isNew and/or need to reference the entity ID in some way.

[pabloelcolombiano]

In that case, how about:

1. $article = ArticleFactory::make()->persist(); // persists an entity. No Model events are triggered.
2. Manipulate $article at your convenience, e.g. $article->is_published = true;
3. $ArticlesTable->save($article); // Save the article the CakePHP regular way.
4. Events Model including beforeSave and afterSave are triggered by the action in point 3.
5. Assert that you obtained what you expected.

Does this work in your new scenario?

The isNew = true case is covered by the first scenario (described in the comment above). The isNew = false is covered by the scenario in the present comment.

[Zuluru]

Yes, this is what I'm doing. But with the recent changes to disable events, step 4 sometimes doesn't happen, because the table has had all those events removed. If I run only the test in question, then it works fine; if I run the entire test suite, then something earlier in the run disables them and they don't get added back. Which is weird, because I thought that Cake makes a fresh table registry for each test.

[pabloelcolombiano]

The package does not disable any events. If the method ::make is used, a different TableLocator, cloned on the regular TableLocator is used. But at no time the events of a TableRegistry get removed or added.

The test here illustrates that what you suspect to happen through the package does not happen: https://github.com/pakacuda/cakephp-fixture-factories/blob/master/tests/TestCase/ORM/Locator/FactoryTableLocatorTest.php#L116

I do not know the context in which the issue occurs, but I would suggest isolating the case you are having and possibly propose a test case showing that the package would fail.