T&A: Export/Import Refactoring

[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