Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

API CHANGE: Don't have any instance caching in singleton(), rely on I…

…njector for this.
  • Loading branch information...
commit 114ebb695342e0ebdd2acd8c9240e3f79cea6027 1 parent f65a7c6
Sam Minnée sminnee authored
9 core/Core.php
View
@@ -365,15 +365,10 @@ function getClassFile($className) {
* @return Object
*/
function singleton($className) {
- global $_SINGLETONS;
+ if($className == "Config") user_error("Don't pass Config to singleton()", E_USER_ERROR);
if(!isset($className)) user_error("singleton() Called without a class", E_USER_ERROR);
if(!is_string($className)) user_error("singleton() passed bad class_name: " . var_export($className,true), E_USER_ERROR);
- if(!isset($_SINGLETONS[$className])) {
- if(!class_exists($className)) user_error("Bad class to singleton() - $className", E_USER_ERROR);
- $_SINGLETONS[$className] = Injector::inst()->get($className);
- if(!$_SINGLETONS[$className]) user_error("singleton() Unknown class '$className'", E_USER_ERROR);
- }
- return $_SINGLETONS[$className];
+ return Injector::inst()->get($className);
}
function project() {
5 core/Object.php
View
@@ -466,6 +466,7 @@ public static function add_extension($class, $extension) {
}
Config::inst()->update($class, 'extensions', array($extension));
+ Injector::inst()->unregisterAllObjects();
// load statics now for DataObject classes
if(is_subclass_of($class, 'DataObject')) {
@@ -496,8 +497,7 @@ public static function remove_extension($class, $extension) {
Config::inst()->remove($class, 'extensions', Config::anything(), $extension);
// unset singletons to avoid side-effects
- global $_SINGLETONS;
- $_SINGLETONS = array();
+ Injector::inst()->unregisterAllObjects();
Marcus
nyeholt added a note

What are the 'side-effects' that need a clearing out of all the objects here (and in add_extension)?

I'm concerned this could have some potentially nasty side-effects itself; the case I'm thinking of is

  • ObjectA is bound to singletonA
  • ObjectB is bound to singletonA
  • add_extension clears the injector of singletonA
  • ObjectC is bound to singletonA
  • Now ObjectA->singletonA !== ObjectC->singletonA

As far as I can tell off the top of my head this doesn't cause any problems at the moment, but it's something that could cause some headscratching for someone down the line. Clearing all objects feels like a work around for something occurring instead of solving the root cause

Andrew Short
ajshort added a note

I've run into some pretty big problems with this - if you register a named service, it's cleared every time an extension is added. What were the reasons for adding this?

Sam Minnée Owner
sminnee added a note

The problem was that the cached object didn't have the relevant extension. We probably need a way of regenerating the created object without removing registered objects. Alternatively, the add_extension call could update the cached instance in the injector, but that might get weird.

Another way of side-stepping the problem would be to get pickier about when add_extension could be called, and ensure that extensions were applied before other activities such as injector registrations.

We're still going to have a catch-22 if you register an specific object instance and then add an extension to that object's class. Forcing extensions to be applied prior to everything else would side-step that.

Marcus
nyeholt added a note

Alternatively, the add_extension call could update the cached instance in the injector, but that might get weird.

Without digging deeper into the murk of how extensions are managed right now, I'd see that as the more appropriate way of approaching things in the interim.

With extensions in general, is it common to have add_extension being called during application execution as opposed to during the bootstrapping process via _config (or triggered from a method call in _config)?

Hamish Friedlander Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
// unset some caches
$subclasses = ClassInfo::subclassesFor($class);
@@ -541,7 +541,6 @@ public function __construct() {
if($extensionClasses = ClassInfo::ancestry($this->class)) foreach($extensionClasses as $class) {
if(in_array($class, $notExtendable)) continue;
-
if($extensions = Config::inst()->get($class, 'extensions', Config::UNINHERITED)) {
foreach($extensions as $extension) {
// Get the extension class for this extension
5 dev/SapphireTest.php
View
@@ -299,8 +299,7 @@ function setUpOnce() {
}
// clear singletons, they're caching old extension info
// which is used in DatabaseAdmin->doBuild()
- global $_SINGLETONS;
- $_SINGLETONS = array();
+ Injector::inst()->unregisterAllObjects();
// Set default timezone consistently to avoid NZ-specific dependencies
date_default_timezone_set('UTC');
@@ -789,9 +788,7 @@ static function delete_all_temp_dbs() {
function resetDBSchema($includeExtraDataObjects = false) {
if(self::using_temp_db()) {
// clear singletons, they're caching old extension info which is used in DatabaseAdmin->doBuild()
- global $_SINGLETONS;
Injector::inst()->unregisterAllObjects();
- $_SINGLETONS = array();
$dataClasses = ClassInfo::subclassesFor('DataObject');
array_shift($dataClasses);
7 model/DataQuery.php
View
@@ -123,7 +123,8 @@ function initialiseQuery() {
$this->query->setFrom("\"$baseClass\"");
- singleton($this->dataClass)->extend('augmentDataQueryCreation', $this->query, $this);
+ $obj = Injector::inst()->get($baseClass);
+ $obj->extend('augmentDataQueryCreation', $this->query, $this);
}
function setQueriedColumns($queriedColumns) {
@@ -225,7 +226,9 @@ function getFinalisedQuery($queriedColumns = null) {
$query->selectField("CASE WHEN \"$baseClass\".\"ClassName\" IS NOT NULL THEN \"$baseClass\".\"ClassName\" ELSE '$baseClass' END", "RecordClassName");
// TODO: Versioned, Translatable, SiteTreeSubsites, etc, could probably be better implemented as subclasses of DataQuery
- singleton($this->dataClass)->extend('augmentSQL', $query, $this);
+
+ $obj = Injector::inst()->get(ClassInfo::baseDataClass($this->dataClass));
+ $obj->extend('augmentSQL', $query, $this);
$this->ensureSelectContainsOrderbyColumns($query);
4 tests/core/ObjectTest.php
View
@@ -11,9 +11,7 @@ class ObjectTest extends SapphireTest {
function setUp() {
parent::setUp();
-
- global $_SINGLETONS;
- $_SINGLETONS = array();
+ Injector::inst()->unregisterAllObjects();
}
function testHasmethodBehaviour() {
Marcus

What are the 'side-effects' that need a clearing out of all the objects here (and in add_extension)?

I'm concerned this could have some potentially nasty side-effects itself; the case I'm thinking of is

  • ObjectA is bound to singletonA
  • ObjectB is bound to singletonA
  • add_extension clears the injector of singletonA
  • ObjectC is bound to singletonA
  • Now ObjectA->singletonA !== ObjectC->singletonA

As far as I can tell off the top of my head this doesn't cause any problems at the moment, but it's something that could cause some headscratching for someone down the line. Clearing all objects feels like a work around for something occurring instead of solving the root cause

Andrew Short

I've run into some pretty big problems with this - if you register a named service, it's cleared every time an extension is added. What were the reasons for adding this?

Sam Minnée

The problem was that the cached object didn't have the relevant extension. We probably need a way of regenerating the created object without removing registered objects. Alternatively, the add_extension call could update the cached instance in the injector, but that might get weird.

Another way of side-stepping the problem would be to get pickier about when add_extension could be called, and ensure that extensions were applied before other activities such as injector registrations.

We're still going to have a catch-22 if you register an specific object instance and then add an extension to that object's class. Forcing extensions to be applied prior to everything else would side-step that.

Marcus

Alternatively, the add_extension call could update the cached instance in the injector, but that might get weird.

Without digging deeper into the murk of how extensions are managed right now, I'd see that as the more appropriate way of approaching things in the interim.

With extensions in general, is it common to have add_extension being called during application execution as opposed to during the bootstrapping process via _config (or triggered from a method call in _config)?

Hamish Friedlander
Please sign in to comment.
Something went wrong with that request. Please try again.