Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A few bug fixes and changes to make the injector play better with existing code. #477

Merged
merged 1 commit into from
May 22, 2012
Merged

A few bug fixes and changes to make the injector play better with existing code. #477

merged 1 commit into from
May 22, 2012

Conversation

nyeholt
Copy link
Contributor

@nyeholt nyeholt commented May 22, 2012

Driven by the change for singleton and strong_create to use dependency injector.

BUGFIX Versioned's constructor doesn't provide suitable defaults. Previously a bug/feature in singleton, where it would pass null,true as params to strong_create, which would then get passed through as params to Versioned's constructor, meant that the code still executed fine (as was set to something that wasn't an array, so the null and true were instead taken as args). The fact that the usage of singleton(Versioned) never really used the classes code, purely for value lookup, meant that this never propagated errors. I've now switched singleton() to use the injector for retrieving values, which means these dud values are no longer passed through

CHANGE Given that Config::inst is an implementation of the singleton pattern itself, I've removed the extra call to singleton(). A side effect of this is that it gets around a possibly nasty circular reference with the dependency injector (which relies on the config object); in future, this dependency structure should really be structured from the DI directly.

…viously a bug/feature in singleton, where it would pass null,true as params to strong_create, which would then get passed through as params to Versioned's constructor, meant that the code still executed fine (as was set to something that wasn't an array, so the null and true were instead taken as args). The fact that the usage of singleton(Versioned) never really used the classes code, purely for value lookup, meant that this never propagated errors. I've now switched singleton() to use the injector for retrieving values, which means these dud values are no longer passed through

CHANGE Given that Config::inst is an implementation of the singleton pattern itself, I've removed the extra call to singleton(). A side effect of this is that it gets around a possibly nasty circular reference with the dependency injector (which relies on the config object); in future, this dependency structure should really be structured from the DI directly.

MINOR Change singleton and strong_create to use dependency injector
halkyon pushed a commit that referenced this pull request May 22, 2012
A few bug fixes and changes to make the injector play better with existing code.
@halkyon halkyon merged commit 627def6 into silverstripe:master May 22, 2012
@halkyon
Copy link
Contributor

halkyon commented May 22, 2012

Sorry Marcus, I've had to revert this because it broke a lot of unit tests. Could you please look into that and open another pull request?

@nyeholt
Copy link
Contributor Author

nyeholt commented May 22, 2012

Hmm, which tests? I wonder if my version was slightly out of sync - is there a log somewhere?

@halkyon
Copy link
Contributor

halkyon commented May 22, 2012

@nyeholt
Copy link
Contributor Author

nyeholt commented May 22, 2012

So locally all tests are passing, so I'll need to check whether there's been some commits since my changes that have gone a bit haywire. The only other difference I can see is that I'm running phpunit 3.6 and that's running 3.5, but I doubt that's causing the issue. Will look into it a bit later today after a couple of meetings

@nyeholt
Copy link
Contributor Author

nyeholt commented May 22, 2012

Is it possible for someone to apply the following patch (https://gist.github.com/2772403) to a local sapphire instance and run their tests locally? I'm not getting any of the errors as output by the build bot, and it'd be good to figure out if someone else can duplicate the failures :)

@halkyon
Copy link
Contributor

halkyon commented May 23, 2012

I get the same issues locally as the buildbot.

Here's one of the failures:

DataExtensionTest::testAddExtensionLoadsStatics
Couldn't run query: UPDATE "DataExtensionTest_Player" SET "ClassName" = 'DataExtensionTest_Player', "Status" = 'Goalie', "Name" = 'Joe', "DateBirth" = '1990-05-10', "Address" = '123 somewhere street', "LastEdited" = '2012-05-23 01:29:04', "Created" = '2012-05-23 01:29:04' where "ID" = 1 | no such column: Status
user_error(Couldn't run query: UPDATE "DataExtensionTest_Player" SET "ClassName" = 'DataExtensionTest_Player', "Status" = 'Goalie', "Name" = 'Joe', "DateBirth" = '1990-05-10', "Address" = '123 somewhere street', "LastEdited" = '2012-05-23 01:29:04', "Created" = '2012-05-23 01:29:04' where "ID" = 1 | no such column: Status,256)
Database.php:658

SS_Database->databaseError(Couldn't run query: UPDATE "DataExtensionTest_Player" SET "ClassName" = 'DataExtensionTest_Player', "Status" = 'Goalie', "Name" = 'Joe', "DateBirth" = '1990-05-10', "Address" = '123 somewhere street', "LastEdited" = '2012-05-23 01:29:04', "Created" = '2012-05-23 01:29:04' where "ID" = 1 | no such column: Status,256)
SQLite3Database.php:181

SQLite3Database->query(UPDATE "DataExtensionTest_Player" SET "ClassName" = 'DataExtensionTest_Player', "Status" = 'Goalie', "Name" = 'Joe', "DateBirth" = '1990-05-10', "Address" = '123 somewhere street', "LastEdited" = '2012-05-23 01:29:04', "Created" = '2012-05-23 01:29:04' where "ID" = 1)
Database.php:606

SS_Database->manipulate(Array)
DB.php:174

DB::manipulate(Array)
DataObject.php:1155

DataObject->write()
DataExtensionTest.php:88

@halkyon
Copy link
Contributor

halkyon commented May 23, 2012

Tests are run using php framework/cli-script.php dev/tests/all flush=all

I'm using PHPUnit 3.6 as well, so it can't be that. :)

@nyeholt
Copy link
Contributor Author

nyeholt commented May 23, 2012

Ah, you're running using sqlite? Lemme check that then...

@halkyon
Copy link
Contributor

halkyon commented May 23, 2012

I think it's failing in MySQL too...

@nyeholt
Copy link
Contributor Author

nyeholt commented May 23, 2012

Yeah I think it's to do with flush=all in combination with tests/all and how everything gets re-inited. I can run the individual test class on its own and it completes successfully, but run as dev/tests/all flush=all and it'll fail.

bergice pushed a commit to open-sausages/silverstripe-framework that referenced this pull request Nov 18, 2018
…bs-nav

BUG Don't add redundant href="#" to tabs
sabina-talipova pushed a commit to creative-commoners/silverstripe-framework that referenced this pull request Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants