Dependency Graph

Dependency Graph
related to related to child of child of duplicate of duplicate of

View Issue Details

IDProjectCategoryView StatusLast Update
17261Development Otherpublic2021-06-07 20:20
Reportergabrieljenik Assigned Togabrieljenik  
PrioritynoneSeverityminor 
Status reviewResolutionopen 
Summary17261: Create new code artifacts to handle QuestionAttributes fetching (& saving)
Description## Olle:

There's a design challenge in the application/models/services/QuestionAttributeHelper.php now. All the previous methods have no dependencies, but the new method depends on (static) methods from model QuestionAttribute and QuestionTheme. So should they be in the same class? Or should the previous methods be moved outside a class instead, but in a namespace? (Might be OK if they have neither state nor dependencies, contradicting what I said earlier.)

If you do constructor injection, you have to inject the dependencies even if they are not used (or do nullable dependencies).
Or you need a new helper: QuestionAttributeValuesHelper. :|


## Gabriel:

I would try to follow the rule:

DB Access, XML as a Source: Model
Handling information: Service / Helper
XML handling, like installation or scanning: Service / Helper
If I had to do these again, I would make the Attribute to be an abstract class with 3 implementation classes depending on the source: DB, XML and Plugin Events. I think all of them could be model-like classes (all sources of information) sharing part of their interfaces.

Then a QuestionAttributes Collection-Like class coordinating all of them (probably holding some of the code which is now on the service class).

The Service class would be then much slimer.

## Olle

Yes, this is the problem. We have one QuestionAttribute class for db, but as you say, since LS4, question attributes have multiple storage points, not only database.

If we have one domain model with two storage strategies, we could consider not using ActiveRecord, but maybe in fact the repository pattern? https://designpatternsphp.readthedocs.io/en/latest/More/Repository/README.html

Maybe it's too complex and overkill, but the point is to separate the domain model from whatever storage strategy is used. ActiveRecord is not adapted for that, since it assumes a database storage (and not file/XML).
Additional Information## Olle

```
$qf = new QuestionAttributeFetcher();
$qf->setTheme($theme);
$qf->setQuestionId($qid);
$attrs = $qf->fetch(); // Calls plugin event and gets XML attributes
$attrsWithValues = $qf->populateValues($attrs);
```


## Denis
then need 2 function : one to get whole attributes definition + one for values (from DB ?)

what's the difference between whole definition and partial definition...?
Whole:
- core attributes
- theme attribute
- plugin attribute
- (future) survey theme attribute
- (future) extend em attribute
- (future) i don't klnow attribute
- partial : see all issue questiontheme and plugin with question attribute since 4.0.0 : we need to be sure to have whole attribute … and it's not the case in 4.0, 4.1, 4.2, 4.3, 4.4 …

## Olle

Yes, and if setTheme is used, it would get attributes from core and theme (and plugin, by default). Can you amend my code with the changes you need?

## Denis

> Yes, and if `setTheme` is used, it would get attributes from core and theme (and plugin, by default). Can you amend my code with the changes you need?

I don't want to amend anytghing , i just answer about:

> Another thought, related to function composition: If we already have the question attributes, maybe it would be enough to do "populateValues(getQuestionAttributes());"?

Why not, but if we have a `populateValues` we need a `getAllAttributesDefinition` for a qid.

Else , about :

```
$qf = new QuestionAttributeFetcher();
$qf->setTheme($theme);
$qf->setQuestionId($qid);
$attrs = $qf->fetch(); // Calls plugin event and gets XML attributes
$attrsWithValues = $qf->populateValues($attrs);
```

Since theme are not in question :

```
$qf = new QuestionAttributeFetcher();
$qf->setQuestionId($qid);
$qf->setTheme();
$attrs = $qf->fetch(); // Calls plugin event and gets XML attributes
$attrsWithValues = $qf->populateValues($attrs);
```

maybe

```
/**
* set the theme for current qid
* @param string $theme, if null get current one by database
* @return void
*/
public function setTheme($theme = null) {

}
```
TagsNo tags attached.

Relationships

related to 16669 resolvedgabrieljenik Bug reports getQuestionAttributes function don't get the plugins attribute 
related to 17262 resolvedgabrieljenik Development  Create new getQuestionAttributes() endpoint for LimesurveyAPI which is used by plugins 
related to 17355 assigned Development  Code cleanup after attributeFetcher service 

Activities

gabrieljenik

gabrieljenik

2021-05-07 16:56

manager   ~64291

Last edited: 2021-05-07 16:57

View 2 revisions

Approach:

- QuestionAttributeProvider abstract class
  coreQuestionAttributeProvider, themeQuestionAttributeProvider, pluginQuestionAttributeProvider

- QuestionAttributeFetcher, only fetches definitions from the providers

- QuestionAttributeValueFetcher, fetches values from DB and merge it with defaults from definitions.
  This would be similar to the current service class. We could use the current one actually, without changing the name.

- QuestionAttribute model should be called QuestionAttributeValue.
  Wouldn't rename it so far, but would be great. This model would be called from QuestionAttributeValueFetcher

  Still, some of its methods should be extracted out:
  + getQuestionAttributesSettings
  + getAdvancedAttributesFromXml
  + getGeneralAttibutesFromXml
  + getOwnQuestionAttributesViaPlugin
  + addAdditionalAttributesFromExtendedTheme

  This should be moved either to a QuestionAttributeProvider implementation or to a service helper.

What do you think?
gabrieljenik

gabrieljenik

2021-05-10 16:16

manager   ~64330

First draft: https://github.com/gabrieljenik/LimeSurvey/commit/fafa2ac1e1db587e59ac507c5757787c08b0dbca
@ollehar comments?
ollehar

ollehar

2021-05-10 16:18

administrator   ~64331

Cool! Don't have time to read today, but tomorrow (I hope).
gabrieljenik

gabrieljenik

2021-05-26 18:24

manager   ~64572

PR: https://github.com/LimeSurvey/LimeSurvey/pull/1893

Ready for reviewal

Issue History

Date Modified Username Field Change
2021-04-20 17:34 gabrieljenik New Issue
2021-04-20 17:34 gabrieljenik Status new => assigned
2021-04-20 17:34 gabrieljenik Assigned To => gabrieljenik
2021-04-20 17:34 gabrieljenik Issue generated from: 16669
2021-04-20 17:34 gabrieljenik Relationship added related to 16669
2021-04-20 17:34 gabrieljenik Project Bug reports => Development
2021-04-20 17:34 gabrieljenik Category Question editor => Other
2021-04-20 17:38 gabrieljenik Issue cloned: 17262
2021-04-20 17:38 gabrieljenik Relationship added related to 17262
2021-04-27 09:55 c_schmitz Product Version 4.3.16 =>
2021-05-07 16:56 gabrieljenik Note Added: 64291
2021-05-07 16:57 gabrieljenik Note Edited: 64291 View Revisions
2021-05-10 16:16 gabrieljenik Note Added: 64330
2021-05-10 16:18 ollehar Note Added: 64331
2021-05-26 18:24 gabrieljenik Note Added: 64572
2021-05-26 18:24 gabrieljenik Status assigned => review
2021-06-07 20:20 gabrieljenik Issue cloned: 17355
2021-06-07 20:20 gabrieljenik Relationship added related to 17355