From cd1c9bbf7271f4c2417dc1694c6b8b4a5fdd4e3e Mon Sep 17 00:00:00 2001 From: K Foster Date: Wed, 21 Feb 2024 16:41:39 +0000 Subject: [PATCH] Dev: Fix PHP-DI CActiveRecord factory (#3731) * Dev: Update DI config * Dev: Remove debug code --- application/libraries/DI.php | 47 +++++++------------ .../AnswersService.php | 28 +++++++---- .../AnswersServiceTest.php | 41 +++------------- 3 files changed, 42 insertions(+), 74 deletions(-) diff --git a/application/libraries/DI.php b/application/libraries/DI.php index b6dda5662ed..39fbd491397 100644 --- a/application/libraries/DI.php +++ b/application/libraries/DI.php @@ -2,6 +2,7 @@ namespace LimeSurvey; +use DI\ContainerBuilder; use CActiveRecord; use LSYii_Application; use LimeSurvey\PluginManager\PluginManager; @@ -41,34 +42,22 @@ public static function getContainer() */ public static function makeContainer() { - $container = new \DI\Container; - - // Type hinting on a Yii model / active-record class should return its - // - static instance e.g Survey::model(). It is not possible to type hint - // - on this, so instead we configure the container to return the correct - // - object based on the call time class name, whenever we type hint on - // - CActiveRecord or anything that extends CActiveRecord - $container->set(CActiveRecord::class, function (CActiveRecord $entry) { - $class = $entry->getName(); - return $class::model(); - }); - - $container->set(LSYii_Application::class, function () { - return App(); - }); - - $container->set(PluginManager::class, function () { - return App()->getPluginManager(); - }); - - $container->set(CHttpSession::class, function () { - return App()->session; - }); - - $container->set(CDbConnection::class, function () { - return App()->db; - }); - - return $container; + $builder = new ContainerBuilder(); + $builder->addDefinitions([ + LSYii_Application::class => function () { + return App(); + }, + PluginManager::class => function () { + return App()->getPluginManager(); + }, + CHttpSession::class => function () { + return App()->session; + }, + CDbConnection::class => function () { + return App()->db; + } + ]); + + return $builder->build(); } } diff --git a/application/models/services/QuestionAggregateService/AnswersService.php b/application/models/services/QuestionAggregateService/AnswersService.php index b7173138eab..c03c37a9c30 100644 --- a/application/models/services/QuestionAggregateService/AnswersService.php +++ b/application/models/services/QuestionAggregateService/AnswersService.php @@ -25,6 +25,17 @@ class AnswersService { use ValidateTrait; + private Answer $modelAnswer; + private AnswerL10n $modelAnswerL10n; + + public function __construct( + Answer $modelAnswer, + AnswerL10n $modelAnswerL10n + ) { + $this->modelAnswer = $modelAnswer; + $this->modelAnswerL10n = $modelAnswerL10n; + } + /** * Based on QuestionAdministrationController::actionSaveQuestionData() * @@ -105,8 +116,7 @@ private function storeAnswerOption( // We use the container to create a model instance // allowing us to mock the model instance via // container configuration in unit tests - $answer = DI::getContainer() - ->make(Answer::class)->findByPk($answerId); + $answer = $this->modelAnswer->findByPk($answerId); if (!$answer) { $answer = DI::getContainer() ->make(Answer::class); @@ -157,13 +167,12 @@ private function storeAnswerOption( */ private function storeAnswerL10n(Answer $answer, $language, $text) { - $l10n = DI::getContainer() - ->make(AnswerL10n::class)->findByAttributes( - [ - 'aid' => $answer->aid, - 'language' => $language - ] - ); + $l10n = $this->modelAnswerL10n->findByAttributes( + [ + 'aid' => $answer->aid, + 'language' => $language + ] + ); if (!$l10n) { $l10n = DI::getContainer()->make(AnswerL10n::class); } @@ -172,7 +181,6 @@ private function storeAnswerL10n(Answer $answer, $language, $text) 'language' => $language, 'answer' => $text ]); - if (!$l10n->save()) { throw new PersistErrorException( gT('Could not save answer option') diff --git a/tests/unit/services/QuestionAggregateService/AnswersServiceTest.php b/tests/unit/services/QuestionAggregateService/AnswersServiceTest.php index 99aa08acdaf..07c701ef7e7 100644 --- a/tests/unit/services/QuestionAggregateService/AnswersServiceTest.php +++ b/tests/unit/services/QuestionAggregateService/AnswersServiceTest.php @@ -33,9 +33,6 @@ public function testSaveThrowsExceptionBadRequestOnMissingCode() BadRequestException::class ); - $modelQuestionAttribute = Mockery::mock(QuestionAttribute::class) - ->makePartial(); - $question = Mockery::mock(Question::class) ->makePartial(); $question->shouldReceive('settAttributes') @@ -50,9 +47,8 @@ public function testSaveThrowsExceptionBadRequestOnMissingCode() ); $question->shouldReceive('deleteAllAnswers')->once(); - - $answersService = new AnswersService( - $modelQuestionAttribute + $answersService = DI::getContainer()->get( + AnswersService::class ); $answersService->save($question, [ @@ -71,9 +67,6 @@ public function testSaveThrowsExceptionPersistErrorOnQuestionSaveAnswerFailure() PersistErrorException::class ); - $modelQuestionAttribute = Mockery::mock(QuestionAttribute::class) - ->makePartial(); - $question = Mockery::mock(Question::class) ->makePartial(); $question->shouldReceive('settAttributes') @@ -94,15 +87,8 @@ public function testSaveThrowsExceptionPersistErrorOnQuestionSaveAnswerFailure() $answer->shouldReceive('save') ->andReturn(false); - DI::getContainer()->set( - Answer::class, - function () use ($answer) { - return $answer; - } - ); - - $answersService = new AnswersService( - $modelQuestionAttribute + $answersService = DI::getContainer()->get( + AnswersService::class ); $answersService->save($question, [ @@ -121,9 +107,6 @@ public function testSaveThrowsExceptionPersistErrorOnQuestionSaveAnswerL10nFailu PersistErrorException::class ); - $modelQuestionAttribute = Mockery::mock(QuestionAttribute::class) - ->makePartial(); - $question = Mockery::mock(Question::class) ->makePartial(); $question->shouldReceive('settAttributes') @@ -143,27 +126,15 @@ public function testSaveThrowsExceptionPersistErrorOnQuestionSaveAnswerL10nFailu $answer->shouldReceive('settAttributes'); $answer->shouldReceive('save') ->andReturn(true); - DI::getContainer()->set( - Answer::class, - function () use ($answer) { - return $answer; - } - ); $l10n = Mockery::mock(AnswerL10n::class) ->makePartial(); $l10n->shouldReceive('settAttributes'); $l10n->shouldReceive('save') ->andReturn(false); - DI::getContainer()->set( - AnswerL10n::class, - function () use ($l10n) { - return $l10n; - } - ); - $answersService = new AnswersService( - $modelQuestionAttribute + $answersService = DI::getContainer()->get( + AnswersService::class ); $answersService->save($question, [