[lukas-heinrich]

Hi everyone,

this PR concerns the refactoring of export/import in the Test and TestQuestionPool components.

Objective

This refactoring reintegrates the test export into the intended export process of ILIAS. In detail, the PR pursues the following objectives:

• The export works both for the current structure and after the migration to the answer types.
• A clear separation of responsibilities makes it possible to add further export options and formats in the future without having to modify the entire export process.
• The export of the Test component is migrated into the structure and interfaces of the global export component. This avoids the parallel infrastructure that previously existed.
• After integration into the ILIAS core, the previous export format will continue to be supported for at least one major version. To this end, compatibility between the previous and the new format is established.

Core idea

The export process is divided into two clearly separated responsibilities:

1. Gathering all content and metadata required for the export. This data initially exists in different forms (for example as data objects, nested array structures, or exports from other components) and is transferred into an independent, simplified structure that is decoupled from the specific export option.
2. These data structures are then converted into the respective target format and persisted in one or more files.

This separation ensures that the export can be implemented independently of the output format, so that not every exportoption needs to build its own process.

The complete concept, including more detailed explanations, is attached here:
Konzeption_ Test Export.pdf

[kergomard]

Thank you very much for the PR @lukas-heinrich

This is a huge improvement! ...even if my list of "complaints" is rather long, from where I stand, they deal with "technical details".

Please see this as a first review, so that we can start iterating. I tried to be thorough, but this is quite a bit to process. I added some details just because they registered with my senses, but mostly it will be about the general structure as this is a lot to go through.

First a very general observation:
For me the deep structure of this code on a first read looks sound. Thanks again for this! ...but it also looks like it tries very hard to be fancy and I think this reduces readability by a lot and leads to code that lies, like here.
For me this feels a lot like you tried to build a generalizable infrastructure for a case were we only need a specific solution. I understand that this seems to come from Symphony and there it might have its reasons and applications, here I do not think it does.
Simplifying the code for me is a rather hard requirement and you would need to show me that life gets really, really hard if we don't simplify it, to sway me. I like boring. This observation will guide a lot of my feedback below.
Additionally, all these Generics, Attributes and similar concepts in PHP look very much tagged on. The moment you need a ReflectionClass to run your code, in my eyes, things are just wrong. ReflectionClasses are for debugging and nowhere else. I think we should get rid of all of it again. I do not believe, as far as I can see, that there is a single thing here that you cannot easily do with good, old, boring code.
E.g. the use of Attributes here seems to only by a solution for a temporary issue as later on we will get the transformations right out Components, but for now you can simply require a Builder that you can then collect and that must have an empty constructor. We can then simply get rid of it as soon as we can use the Components directly.

So I would first ask you:

• Is there an absolute reason we should not get rid of the Generics, the Attributes, most mixed types and most of the type declarations in Doc-Blocks?

One structural thing, though, and this might rather be a biggy:

• Normalizable(see below for nomenclature), cannot contain a function fromNormalized. This MUST go to a factory. We should never require to have an unconfigured object

Then one request that we need to tackle anyway before we can merge this:

• Do we really need the changes outside of the test? They look rather secondary and like they either provide issue fixes or just be there, because you want to do something your way, instead of the way the creator intended it. We should in any case separate them out into their own PRs if we are absolutely sure we really, really need them.

And then there is the nomenclature. I'm not really happy about that (I do not believe this comes as a surprise):

• I will first simply reiterate and make it very clear that you are not going to get this past me: Do not call anything that is not a normalization a normalization. How are primitives more "normal" than classes in OOP? I think in this case it should be FromImport and ToExport. If you want to make this usable in a more general way (I'm not sure we want, though), then it should be something like ToPrimitives and FromPrimitives (or ToPrintable and FromPrintable or ToExchangeFormat and FromExchangeFormat), but then I think we should also move the Logger to use this infrastructure. By the way: I understand why Symphony calls it normalization as the "normal" there is the API, I still do not think it is right, even in their case.
• Why is ILIAS\TestQuestionPool\ExportImport\Foundation\Contracts\Envelope called "Envelope", but then has toArray and fromArray-functions? I think an envelope is actually opened and closed or (and this is already stretching it) packed and unpacked. I think this should be called ArrayTransformation.
• Please do not call interfaces contracts. they are not really. They are what they say they are: "interfaces". Please remove the whole namespace Contracts and move the corresponding interfaces to the corresponding namespaces. Please also remove all interfaces that are not absolutely necessary, i.e. interfaces that only have one implementation and do not function to reduce the public interface of the class, as far as I can see e.g. Pipeline and Transformations do not provide any added value whatsoever.
• We should not talk about "Solutions". It would have been an option, but there is a lot of code that does not take this approach. In general, what you are exporting with the "Solutions" is an "Attempt".
• Can we get rid of the Bridge\StateHolder? The nomenclature smells (the actual holder is the surrounding class and this is just some kind of "cradle") and it looks utterly unnecessary.
• Please remove all functions from Pipeline that are not used. I think it is quite all of them. And what is left over, as far as I can see, will be a queue, that you can then execute in one go. This cannot be called a Pipeline anyway and the Pipes are no Pipes as far as I can see. A pipeline never processes its contents, it just transports them and the same goes for a pipe. This for me looks over complicated and overengeneered to the max. There even are \Closures returning \Closures, Pipes can also be \Closures and so on. Please simplify as we need something that only solves our problem. So, from where I stand, this should probably be a Queue of Transformations. Could we simply implement this as an Iterator or a derivative thereof? Please move them to a sub-namespace Queue.
• Please synchronize nomenclature inside the Foundation and outside of it. I think it should be Import, Export, Normalize, Serialize, and Queue. Bridge, as far as I can see, should be Export. Contracts, as written, must go.
• Ok, I put this in here, but it is only loosely about nomenclature. Clean-up of Foundation:
  • If I'm understanding this right, we do not need the JSON-implementations, right (at least I didn't find any usages)? So they should leave us. ...and then I'm rather unclear why we have two different XML-Deserializers, but actually no corresponding Serializer. I think we should create symmetry here and only have one Deserializer handling both cases and a corresponding Serializer.
• We have three different concepts with overlapping meaning: Step, State, and Stage. Step and State in Export and Stage in Import, if I'm getting this right. Couldn't we just move everything to a Stage-concept to create some symmetry?

Please make the following changes (this is just a list of random observations):

• Please have a look at the different *.md-files and update them correspondingly (this probably is for later on).
• This here is unclear: Why should you be able to process an ImportObject (whatever that is), if you have read permissions? ...and why do you then get redirected because of a "missing write" even though what you are potentially only missing a read? This looks like a very brown smarty. Additionally: Please do not introduce intermediate variables that are only used once, unless the corresponding code would get really, really hard to read (not the case here, if you strategically employ line breaks).
• Please remove the "@inheritdoc"-comments, but add #[\Override]. I do not think the former does much.
• Please make the ImportContext concrete. Currently we have no idea what is in there and why. Additionally, it will be serialized at a certain point, so you cannot put in whatever you want and one simply needs to know that. Please convert it to JSON instead of serializing it.
• Please do not check for truth where there is none. e.g. here.
• Please do not add use statements for the global namespace (e.g. here).
• Please remove ilAssQuestionLifecycle::getInstance() and make the constructor public. Looks like a very easy win to get rid of a static method.
• Please do not assign variables in conditions and then check for them to be true, e.g. here. Put the assignment into brackets and then make it very clear what you are expecting (in this case !== null, but you should also check for === true in case of bools, as this pattern is very dangerous, but sadly not always easily avoidable).
• I think we should add parenthesis around (!$a instanceof A). I know, I'm mostly against parenthesis, but this is non-obvious and the visual suggests another binding than it actually has. A example can be found here.
• Please make all protected properties and functions private unless you are absolutely sure you want to allow somebody to overwrite them (which should very rarely be the case).
• I do not think the Id-Envelope is right. It is completely random: Why should an ID be a bool and what do the different things you can put in here have in common? Why would we want an object-string (as far as I can see, it is not even used now)? I think we can simply get rid of this.
• Carrys shouldn't expose their properties. Please provide corresponding getters or even better: More complex functions that provide the information that is really needed outside.

Ok, I will stop here as this is already quite a lot. Maybe we should have a call to expedite the further development? ...otherwise you will also write a book, to really answer everything here.

Thanks again for all the work and best,
@kergomard

[lukas-heinrich]

Hi @kergomard ,

Thank you very much for the detailed review and your suport on this major refactoring. As I mentioned in our correspondence via discord, I support most of the suggestions and have implemented them accordingly. Below, I’d like to address a few open points:

Do we really need the changes outside of the test? They look rather secondary and like they either provide issue fixes or just be there, because you want to do something your way, instead of the way the creator intended it. We should in any case separate them out into their own PRs if we are absolutely sure we really, really need them.

That is absolutely correct. When I rebased the code, I noticed that the changes outside the Test and TestQuestionPool components were removed, since they had apparently already been integrated into trunk elsewhere. Accordingly, this PR now only includes changes to our components.

Can we get rid of the Bridge\StateHolder? The nomenclature smells (the actual holder is the surrounding class and this is just some kind of "cradle") and it looks utterly unnecessary.

I understand the reluctance to use this pattern here. In my pov there is unfortunately no other way. The export component from the core creates two ilXmlExporter instances (getMinimalComponentExporter for tail dependencies, getComponentExporter for the XML representation). If the state were stored in a property of this class, it would be lost upon the second instantiation. For this reason, I created the StateHolder to manage the state. In the document I attached at the very top, I’ve documented this issue in detail within the section “Bridge: XML Exporter ↔ Exporter Container".

We have three different concepts with overlapping meaning: Step, State, and Stage. Step and State in Export and Stage in Import, if I'm getting this right. Couldn't we just move everything to a Stage-concept to create some symmetry?

In my opinion, the two concepts ExportState and ImportStage cannot be combined. I’m leaving ExportStep out of this discussion, since it’s merely an enum used to describe the export process in a type-safe manner.

ExportState encompasses the data for a single export, which is usally one request-lifecycle. After WRITE, it is cleared so that the next export does not inherit any steps, options, or transformations. The sequence is monotonic and closed, meaning there is no external extension or user interaction.

ImportStage is a behavior, visualized by a wizard, that spans multiple requests. A stage has an ID, a label, and process(ImportContext): StageResult. The ImportStageRunner selects the current stage via the session index and can remain on the same stage until the user submits the form.

The lifecycles are opposite. The export clears the state after WRITE so that nothing carries over into the next run. The import writes the context and index to the session so that the same run continues after the next request. A common abstraction would have to support both rules simultaneously and would thus lose the rule for which each type was designed.

I do not think the Id-Envelope is right. It is completely random: Why should an ID be a bool and what do the different things you can put in here have in common? Why would we want an object-string (as far as I can see, it is not even used now)? I think we can simply get rid of this.

I've modified the class so that it now only supports numbers, strings, and UUIDs. However, it remains essential to the functionality. Since each ID is also associated with its corresponding object type, we can specifically collect IDs for certain objects during an export. This is what happens f.e. in CollectUserIds. During normalization, this processor collects all IDs belonging to a user, allowing us to subsequently export the necessary user data.

Thanks again for all the effort you put into this review!
Best
@lukas-heinrich

[dsstrassner]

@kergomard will you look into this PR before your absence, or should @thojou take over?

[kergomard]

Thank you very much for the update @lukas-heinrich !

Now, even I don't feel overwhelmed anymore.

This is just where I got today. I hope I find the time to look at this again next week, but if I don't give more feedback until the end of the next week, feel free to just go ahead.

What I still have issues with:

• [Edit: I think something went wrong here, as above you write, that you have already changed this, but the code does not seem to reflect this:]I still don't like the Id-envelope. Please make it more restrictive a Id has to either be an an int or a Uuid. This allows for stricter typing and I think it also closes a potential entry-point for an exploit as you are building classes from user inputs here. Not sure yet, if there is a way in, but we should make completely sure we cannot even think of one.
• If I understand this correctly, FoundationNormalizerRegistration should simply be NormalizerRegistration ...or do you envision another one existing (I currently do not see a way, as it is final and doesn't implement an interface).
• What I always wonder about: Do we really want to add all this debug-logging? I would not do it: If something goes wrong, the information is never going to be enough and you need to step through the code anyway. Personally I would only log errors and remove everything else. ...it would be something else, if these logs had something to do with security, but they don't. ...this also has the nice side-effect that the code becomes more readable.
• Edit (Sorry! Beween doing a gazillion things, I forgot this comment of yours above. I'm still not 100% comfortable here, I think that there are too many concepts and this usually is a sign that something is not streamlined enough, but as I cannot really give you a concrete suggestion, I will remove this): We still have the three different concepts of a "State", a "Step", and a "Stage". We have a Context in Import, but not in Export, but then we have an Exporter, but not an Importer. Could we please parallelize importing and exporting? If we could reduce the concepts, things would get easier to read.

As said, I've to stop here, but so you at least have some additional feedback.

Thanks again and best,
@kergomard

[kergomard]

Thanks again @lukas-heinrich !

This will be my final comment here and @thojou, you can merge, when you feel ready.

• Thank you very much for giving sensible names to the Factory and Handler shenanigans from Export!
• The coding style is very clean! I would have liked one single little change, but it is not important enough: If we have nested function/method-calls, e.g. array_filter(array_map(...)), I would very much prefer it to only have one call per line. Yes, there are more locs, but from where I stand, they are also easier to read.

One a little bit bigger thing (but not that big):

• Please get rid of ImportStageRunner::buildWorkflow(). If we would really want to do this, we need to find a better way to display it and currently it is displayed inconsistently (why only in the Pool and not in the Test) and has a very limited usability (there are three steps, but the first is outside of our control). If we want to implement this in the future, it should be decided by the import if it should be shown, not by the consumer. Let give this some thought if we deem it important, but let us keep it back for the time being.

A few nitpicks:

• I do not think the cases in StageResultType are well named as they are on multiple levels: Error is a "thing", Complete is probably a "state" (or a command, slightly unclear), Interact is probably a command towards a user. I think it should be Error, RequiresUserInteraction, StageCompleted, and WorkflowCompleted. And if we write it like this, from where I stand, it should also be clear, that WorkflowCompleted is no business of the Stage, but it is up to the context to decide if there is another Stage, so that we would only have Error, RequiresUserInteraction, and CompletedSuccessfully. I think this way we could also get rid of all the comments. There is a slight grammatical mismatch between Error and the others, but I think we should let that stand. Additionally: I would suggest to go with pascal case in Enums (and yes, it was me who started to be inconsistent with this and I'm sorry for that!).
• This here MUST be on a single line (see).
• We should get rid of CollectResources::getResources(). This is too low level, I think. It is only needed once and the correct level there seems to be CollectResources::writeToExportFile(ExportWriter $writer, string $folder). This way we can protect the list from any misuse, additionally nobody needs to know, that the resources are stored as an associative array with the id as key and we can change it, if needed.
• As we are touching ilAssQuestionLifecycle: Shouldn't we make it an Enum? This class looks "unsavory" ;-) .

I now simply stop here and say thanks again as I'm definitely running out of time!

Sorry that I'm a little short on time!

Best,
@kergomard

[lukas-heinrich]

Hi @kergomard ,

Thank you very much for the additional feedback and important remarks. I have integrated the suggested changes accordingly. Regarding the different concepts for export and import, I also agree that these classes are not easily distinguishable from one another based on their names, and it’s not clear why this distinction is necessary and which class contains which information. Therefore, I’ve adjusted the names so that we now have an ExportContext and ExportPhase, and on the other side, an ImportSession + Repository, as well as ImportWizard, ImportStage, and StageResult. I’ve also adjusted StageResultType in accordance with the comment.

I have some notes regarding the last two points from the feedback:

In my perspective, CollectResources::getResources() is necessary because the resource metadata is included in the export via TestExporter::writeMappings. This information is used in TestImporter::importMapping to rename the files according to their new IDs without losing the original relative path and file extension.

To be honest, I don’t see any added value in converting ilAssQuestionLifecycle into an enum, apart from the fact that the validation of valid values would no longer be required due to the type strictness. In my view, the mapping to the LOM lifecycle and translations would still need to be implemented within this enum. This refactoring would also affect files on the caller side that I haven’t touched so far. I’m concerned that this might introduce unintended side effects that wouldn’t be noticed during testing, as they have nothing to do with the export/import functionality. For this reason, I haven’t implemented this change yet.

Otherwise, thank you once again for the valuable feedback and for taking the trouble to grapple with this enormous volume of changes!

Best
@lukas-heinrich