FIX Singleton creation #7850

jonom · 2018-02-08T22:14:19Z

Singleton objects created by the Injector in 3.x always have the populateDefaults() method fired, despite a check in the DataObject constructor intended to prevent this.

The result is that populateDefaults() runs a lot of times that it shouldn't, including during the building of tables, which can easily break a build if you're trying to pull data from a table that isn't ready yet.

Additionally, infinite loops can occur if you try to access another object of the same class from within the populateDefaults() method. Example:

public function populateDefaults()
{
    parent::populateDefaults();
    $first = self::get()->first();
    // Script will timeout
}

If you paste that code example in to the Page class, the CMS won't load at all.

Not sure if in_array('DataObject', ClassInfo::ancestry($class)) or is_subclass_of($class, 'DataObject') would perform better, or if it matters?

dhensby · 2018-02-08T22:28:36Z

tests/model/DataObjectTest.php

@@ -33,6 +33,25 @@ class DataObjectTest extends SapphireTest {
 		'ManyManyListTest_Category',
 	);

+	public function testSingleton() {
+		// Test that populateDefaults() isn't called on singletons, which can lead to SQL errors during build, and endless loops
+		$inst = DataObjectTest_Fixture::create();


might be better to use a dataProvider here

what would be the advantage?

If one test fails it won't block all the other tests.

Do the other tests in this class use dataProviders? Or can you provide an example? I'm not really sure how to proceed.

@jonom see https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.data-providers

I've made the change for you and pushed it up

I've made the change for you and pushed it up

Thanks @dhensby! I didn't even realise you could do that. Were you able to push straight to my branch with your regular credentials or is it more complicated? I'll have to remember that option for the future.

Tests failed after your commit was introduced but passed before. I can't see anything wrong with your commit though so I'm quite confused.

From the docs you linked:

All data providers are executed before both the call to the setUpBeforeClass static method and the first call to the setUp method. Because of that you can't access any variables you create there from within a data provider. This is required in order for PHPUnit to be able to compute the total number of tests.

I guess SapphireTest:setUp() is running after the objects are instantiated?

tractorcow · 2018-02-08T23:56:10Z

control/injector/Injector.php

@@ -551,6 +551,14 @@ protected function instantiate($spec, $id=null, $type = null) {
 			$constructorParams = $spec['constructor'];
 		}

+		// If we're dealing with a DataObject, pass through Singleton flag as second argument
+		if ($type != 'prototype' && in_array('DataObject', ClassInfo::ancestry($class))) {


A lot of binding here to DataObject. How about looking into a DataObjectFactory configured via yaml?

I don't really know what that means. I really want to see this ancient issue fixed but I don't have a lot of time to look further in to it or solve it more elegantly unfortunately. I had thought this would be only for 3.x too but actually glancing at Injector and DataObject in SS4 this is probably still an issue and would need to be merged up.

Ok well, fair enough. I'd probably not want this leaking up into 4.x branch though. 4 should be less concerned about singleton-specific construction.

jonom · 2018-02-09T04:05:56Z

Switched to is_subclass_of to fix Injector test fail

jonom · 2018-02-09T17:44:38Z

tests/model/DataObjectTest.php

+		}
+	}
+
+	public function provideSingletons()


The first two here are not singletons, should that be indicated?

jonom · 2018-02-09T18:05:19Z

tests/model/DataObjectTest.php

@@ -33,6 +33,25 @@ class DataObjectTest extends SapphireTest {
 		'ManyManyListTest_Category',
 	);

+	public function testSingleton() {
+		// Test that populateDefaults() isn't called on singletons, which can lead to SQL errors during build, and endless loops
+		$inst = DataObjectTest_Fixture::create();


I've made the change for you and pushed it up

Thanks @dhensby! I didn't even realise you could do that. Were you able to push straight to my branch with your regular credentials or is it more complicated? I'll have to remember that option for the future.

Tests failed after your commit was introduced but passed before. I can't see anything wrong with your commit though so I'm quite confused.

dhensby · 2018-02-09T18:35:09Z

well, that's fun, using a dataprovider seems to make the test fail :/

jonom · 2018-02-09T19:07:44Z

well, that's fun, using a dataprovider seems to make the test fail :/

@dhensby not sure if you would have seen my comment since it's on an outdate diff:

From the docs you linked:

All data providers are executed before both the call to the setUpBeforeClass static method and the first call to the setUp method. Because of that you can't access any variables you create there from within a data provider. This is required in order for PHPUnit to be able to compute the total number of tests.

I guess SapphireTest:setUp() is running after the objects are instantiated now and something critical happens in there?

tractorcow · 2018-02-11T21:33:46Z

Just make the first argument a closure that creates the singleton. :)

tractorcow · 2018-02-11T21:36:39Z

control/injector/Injector.php

+			if (!count($constructorParams)) {
+				$constructorParams[0] = null;
+			}
+			$constructorParams[1] = true;


Sometimes constructor args are named.

MyObject: class: MyObject constructor: record: [] singleton: true

however that's super edge case. Maybe only apply this fix if $constructorParams are totally empty?

Not a review change request, because it feels edge case enough we can ignore it, but worth thinking about before we modify this in 4.x.

Could we just convert to numeric keys:

$constructorParams = array_values($constructorParams);

The constructor args will be passed by position rather than name right? So I assume it should be safe to discard any non-numeric keys.

It's safer to only supply default constructor params, in this case based on $type != 'prototype' && empty($constructorParams). Trying to merge in based on position / key is guesswork at best.

I don't think we can add a empty($constructorParams) check to this because $constructorParams = [null] would not be considered empty, would it?

Yes but when would we have [null]?

Don't know :) I'm just guessing so I'll trust you on this lol. Have added a commit with a empty($constructorParams) condition.

dhensby · 2018-02-12T10:36:04Z

Also, @jonom does this affect 3.5 too? or just 3.6+?

dhensby · 2018-02-12T10:49:02Z

@jonom

I didn't even realise you could do that. Were you able to push straight to my branch with your regular credentials or is it more complicated? I'll have to remember that option for the future.

It was a feature added a while ago: https://github.com/blog/2247-improving-collaboration-with-forks

By default users that open PRs allow repo owners / contributors the ability to push into that branch until permission is revoked or the PR is closed/merged.

dhensby · 2018-02-12T11:39:59Z

@tractorcow we are green - thanks for the closure suggestion - can you decide if you're happy to merge, please?

jonom · 2018-02-12T20:42:50Z

Also, @jonom does this affect 3.5 too? or just 3.6+?

I'm guessing this bug was introduced in 3.0 or whenever the Injector first became a thing. Sorry I thought we were only patching the latest versions but I can target 3.5 branch instead if you like.

tractorcow · 2018-02-12T21:04:43Z

control/injector/Injector.php

+			if (!count($constructorParams)) {
+				$constructorParams[0] = null;
+			}
+			$constructorParams[1] = true;


It's safer to only supply default constructor params, in this case based on $type != 'prototype' && empty($constructorParams). Trying to merge in based on position / key is guesswork at best.

Ensure DataObject instances are aware they are singletons so functions like populateDefaults() can be skipped. (fixes silverstripe#4878)

tractorcow · 2018-02-13T04:32:36Z

Merge on green.

jonom · 2018-02-13T04:33:17Z

I have a squashed version of this ready for 3.5 if you want me to push that up and change the base? This is targeting 3.6 currently.

tractorcow · 2018-02-13T04:33:38Z

We should merge into 3.5, better.

jonom · 2018-02-13T04:37:37Z

Thanks for your help guys. @dhensby sorry I couldn't work out how to move this to 3.5 and keep a credit on your commit but I figured it was likely to be squashed in to mine anyway.

jonom · 2018-02-13T05:06:04Z

Scrutinizer got a bit confused

dhensby · 2018-02-13T16:31:10Z

OK - so I've merged this up to 3 but it's still an issue in 4 and @tractorcow has asked I don't merge the "fix" up into 4 as we want a less tightly coupled approach in Injector.

@tractorcow I looked into creating a DataObject factory, but the problem is that a factory has to be registered against every class (not just the common parent class) which means there's no consistent or easy way to create a factory for all DataObjects (that I can see). We'd also need to add a third param to the Injector\Factory@create interface to pass along the $type var (SINGLETON vs PROTOTYPE) so our Factory knows how it should be creating the objects.

So bad news is this PR isn't going to get merged up into 4 and all and we'll need a new PR for 4 if we want this fix to make its way up :(

tractorcow · 2018-02-13T20:12:32Z

I really want to get away from singletons needing special arguments to work. I'm highly reluctant to keep supporting this feature going forward.

jonom · 2018-02-13T20:51:16Z

I really want to get away from singletons needing special arguments to work. I'm highly reluctant to keep supporting this feature going forward.

It seems the only use for the $isSingleton arg is to skip firing the populateDefaults() method. That seems pretty important for performance though, and also prevents build errors. If you want to take that argument out I guess you would have to not call populateDefaults() in the constructor. So what then? Do we make people call it manually? If we force people to use the Injector it's not an issue as we could call populateDefaults from there on new prototype DataObjects. But if you want to support the new MyDataObject syntax I can't see how we could handle this case without informing the constructor of whether or not this is a singleton.

Is dropping new support an option? Using the Injector is heavily encouraged, could it be mandated?

tractorcow · 2018-02-13T23:11:18Z

and also prevents build errors.

Only in cases where you have db-dependant populateDefaults() implementations.

We could just guard those with catch (DatabaseException)

tractorcow · 2018-02-13T23:19:19Z

Sure it's an empty catch, but it's a specific guard against a known use case, rather than a global Exception suppressor.

public function populateDefaults()
{
try {
   $first = self::get()->first();
   if ($first) {
       $this->owner->Title = $first->Title . ' new';
   }
} catch (DatabaseException $ex) {
		// guard against errors during db bootstrapping
	}
}

jonom · 2018-02-14T18:21:47Z

So that catch would allow the code to keep running would it? It sounds like a clever solution but it would be nice if devs didn't have to guard against this themselves. I guess if we update the docs on this topic with instructions though that could do the trick.

I'm not actually sure what the reasoning is for DataObject Singletons to not have their defaults populated. Besides preventing build errors, I had assumed perhaps performance but if the whole point of a singleton is that only one for each class exists, executing a little extra code for each one shouldn't be a big concern, and this is what's been happening as long as this bug has existed anyway. I tried to look up the reasoning for the $isSingleton check in the DataObject constructor but it is prehistoric... was introduced in the first commit in this repo.

Maybe the fix we implemented for 3 should be merged up to 4, but in master we could remove the $isSingleton arg from the DataObject constructor and let singletons have their defaults populated?

tractorcow · 2018-02-14T20:05:53Z

Pretty much as you said it... it was more a performance / error prevention step. The solution in my mind is to simply avoid heavy lifting when populating defaults, as well as allowing it to silently ignore DB errors.

Ensure DataObject instances are aware they are singletons so functions like populateDefaults() can be skipped. (fixes #4878) Correctly applies #7850 to the 4.x line This has already been fixed in 3.x

This was referenced Feb 8, 2018

FIX Singleton creation #5154

Closed

FIX Singleton creation #5151

Closed

populateDefaults not working on extensions #4878

Closed

dhensby suggested changes Feb 8, 2018

View reviewed changes

tractorcow suggested changes Feb 8, 2018

View reviewed changes

jonom force-pushed the singleton-fix-3x branch from 94920ca to 82cd8bb Compare February 9, 2018 04:02

dhensby force-pushed the singleton-fix-3x branch 5 times, most recently from 68bd0bc to 7e3bc76 Compare February 9, 2018 12:00

dhensby approved these changes Feb 9, 2018

View reviewed changes

jonom commented Feb 9, 2018

View reviewed changes

tractorcow reviewed Feb 11, 2018

View reviewed changes

dhensby force-pushed the singleton-fix-3x branch from 7e3bc76 to 452ff3f Compare February 12, 2018 10:40

tractorcow suggested changes Feb 12, 2018

View reviewed changes

tractorcow approved these changes Feb 13, 2018

View reviewed changes

FIX DataObject singleton creation

c767e47

Ensure DataObject instances are aware they are singletons so functions like populateDefaults() can be skipped. (fixes silverstripe#4878)

jonom force-pushed the singleton-fix-3x branch from a53eb6e to c767e47 Compare February 13, 2018 04:35

jonom changed the base branch from 3.6 to 3.5 February 13, 2018 04:35

dhensby merged commit 1bec8a5 into silverstripe:3.5 Feb 13, 2018

jonom deleted the singleton-fix-3x branch February 14, 2018 18:25

sminnee mentioned this pull request Oct 15, 2018

[ORM] [2012-08-23] singleton(<DataObject>) calls populateDefaults() - regression? #1575

Closed

sminnee mentioned this pull request Oct 24, 2018

FIX DataObject singleton creation #8516

Merged

kinglozzer mentioned this pull request Jan 23, 2019

BUG: defaults are not inherited in SS3 #8567

Closed

FIX Singleton creation #7850

FIX Singleton creation #7850

Conversation

jonom commented Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow Feb 11, 2018 • edited Loading

Choose a reason for hiding this comment

jonom commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhensby commented Feb 9, 2018

jonom commented Feb 9, 2018

tractorcow commented Feb 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhensby commented Feb 12, 2018

dhensby commented Feb 12, 2018

dhensby commented Feb 12, 2018

jonom commented Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

tractorcow commented Feb 13, 2018

jonom commented Feb 13, 2018 • edited Loading

tractorcow commented Feb 13, 2018

jonom commented Feb 13, 2018

jonom commented Feb 13, 2018

dhensby commented Feb 13, 2018

tractorcow commented Feb 13, 2018

jonom commented Feb 13, 2018

tractorcow commented Feb 13, 2018

tractorcow commented Feb 13, 2018 • edited Loading

jonom commented Feb 14, 2018

tractorcow commented Feb 14, 2018

jonom commented Feb 8, 2018 •

edited

Loading

tractorcow Feb 11, 2018 •

edited

Loading

jonom commented Feb 12, 2018 •

edited

Loading

jonom commented Feb 13, 2018 •

edited

Loading

tractorcow commented Feb 13, 2018 •

edited

Loading