T&A 40791: Superglobals replacement in Test component

[matheuszych]

Superglobal variables such as $_GET or $_POST should not be accessed directly. Instead, use the request and refinery mechanisms. Currently, there are four instances where this change is not feasible due to limitations in the request and refinery systems, but these will be addressed in the future.

Mantis: 40791

[dsstrassner]

Is this internally reviewed by @thojou or @mbecker-databay? If so, I add @kergomard as a reviewer here on GitHub.

[thojou]

@dsstrassner Yes, this PR has been reviewed internally.
@matheuszych As per this commit, the necessary changes to the RequestWrapper have been implemented, so there should be no blockers to proceed with refactoring the sections marked as TODO.

All other refactorings look good to me.
@kergomard Could you please take a look at the changes?

[kergomard]

Thank you very much @matheuszych and @thojou for this PRs!

First: Thank you very much for the boy-scouting! Much appreciated!

And then let us start with a more philosophical observation:

• This PR leaves me unsure in what direction we want to go. You know probably that I was not a big fan of the InternalRequestService and I'm still not happy with the dependence of our RequestDataCollector on the BaseGUIRequest, but from where I stand in this PR we rather worsen the situation: We are increasing the dependece on the HTTP\Service and keeping the one on our RequestDataCollector. Personally I would suggest to stick to the RequestDataCollector and, if you have available time to remove the dependence on the BaseGUIRequest, but this might be something to discuss. I think a case for keeping the RequestDataCollector could be seen here and here and here. I think the abstraction away from direct access to an array-key, helps us to stick to a limited set of keys ...and it would remove duplicate code. Ideally the function should be called getQuestionIdsFromPost() and this would be a clear sign that we should get rid of any uses of it.

Then I would have the following suggestion. You don't have to follow it, please provide an explanation if you don't, though:

• I would suggest to move this here back to depending on the Interface, if there is no very good reason not to. We should also move all other dependencies away from the implementation, if we have an interface, but this is for another time.

Please apply the following changes:

• If we go with this change: Please remove the check for an empty array here. It doesn't do anything anymore.
• Please keep the return types consistent here. I think you should return false and then we get a clear check on the next lines on true, as we know the variable has to always be a boolean, we can just check if ($var)
• Please don't add any doc-blocks, if they are not absolutely necessary, and if you do add one, please make sure no redundant information is present there (so, no @access public). It just creates noise.
• Could you please have a look at this here. I don't think you are checking for $prev to be true, you are checking for $prev to not be null, so we should be explicit about it. Additionally we could get rid of the quadrupel nesting by changing it to if ($key !== $first) {continue;} if ($prev !== null) {....

So, I think, I stop here. If you would like to discuss the whole philosophical issue just ping me, if not, I would suggest to stick to the RequestDataCollector, ...at least in the classes we already have it.

Thanks again and best,
@kergomard

[matheuszych]

@kergomard, @thojou

I addressed the minor issues you mentioned. Further I will look into using the RequestDataCollector (WIP).

[matheuszych]

@kergomard, @thojou

@bidzanaaron and i implemented the requested changes regarding the RequestDataCollector. I believe this PR is ready for a rereview.

[dsstrassner]

@thojou will review internally and @kergomard could approve afterward and merge.

[kergomard]

Thank you very much @matheuszych for the update!

I've only two little change requests:

• Could you please remove the fallback-parameter from the functions on the data_wrapper. My reasoning: The Wrapper should not and cannot know, when the fallback should be used. Is it when the corresponding key is not set? Is it when it is null? is it when it is the empty value of the variable-type? The consumer needs to know what data it needs, thus the wrapper should simply return null if the corresponding key is not set or the value if the key is set.
• And then the very trivial one: Please remove the @throws. We are not planing to react to them in any shape way or form, so we don't care.

As soon as this is done and rebased I will merge asap to avoid any more conflicts.

Thanks again and best,
@kergomard

[kergomard]

Ok, I close this as we have a new version here.

Thank you very much @matheuszych for pursuing this issue further.