UI/DataTable: with additional view control

[nhaagen]

Consumers may add more view controls to the data table;
example and tests are based on #8350, so this will be failing for now.

[nhaagen]

PR #8350 is included, now, this one here does not really make much sense without it.
Furthermore, there are changes in the namesourcing of ViewControls, which of course will affect all VCs.
Basically, it comes down to "a VC will have fixed parameter names". The container's name, however, might still be numbered.
While this of course shortens and prettifies the URL, the main reason is another one: VCs are singletons for their controlled view - it simply does not make sense to have more than one pagination, e.g.
Fixed paramter names make NullControls widely obsolete - there will be no more shifting in param-names when a viewcontrol is omitted due to runtime circumstances (no pagination needed for very little records, no sorting option available, etc...)
Actually, I'm thinking of removing the null control alltogether.

[klees]

Hi @thibsy and @Amstutz,

please have a look into this, LGTM to me now. Please note the comment from @nhaagen, that this already contains #8350.

If you need some more intro to the code we can maybe discuss this tomorrow as well.

Kind regards!

[thibsy]

Hi @nhaagen,

Thanks a lot for your contribution to the UI components!

Although this review already covers some implementations details, this is not yet a full technical review; we will do so to a latter point.

About the concrete changes, please answer the following questions:

• Dedicated names: you introduce a new mechanism which does not share the same features with other inputs, because dedicated names will not be enumerated. Can you explain why we need this? Could you also explain, why every view control input receives a dedicated name right in the factory? It feels like this drifts form inputs and view controls farer apart, and I think we should try to avoid this.
• mixed parameters: could you explain why exactly the parameters of the DataRetrieval used for view controls become mixed? Same for Data::withAdditionalParameters(). Do we really need to expect any kind of value there? Why don't we pass this data as an array anymore? If I understood correctly, these parameters should be somewhat similar to data retrieved by Form::getData(), no?
• Data::withAdditionalParameters(): can you explain what this method is used for briefly? Maybe a comment would help.

Kind regards,
@Amstutz and @thibsy

[nhaagen]

Hi @thibsy ,
actually glad you're asking, these were some discussion points with @klees , too

• Dedicated names: you introduce a new mechanism which does not share the same features with other inputs, because dedicated names will not be enumerated. Can you explain why we need this? Could you also explain, why every view control input receives a dedicated name right in the factory? It feels like this drifts form inputs and view controls farer apart, and I think we should try to avoid this.

We currently have enumerated params just like in form-fields; however, the internal fields of the view controls are fixed and not open to consumers; there is no need to enumerate them, and besides the shorter url, expectable parameters are simpler to handle. Foremost with the session storage: if there was, e.g., no pagaination, we filled up the now empty parameter-spaces with null-controls, just to shift to the correct parameter name. This seems more brittle than just dedicating names. Enumeration takes place on the container, though; we now have something like "vc0/pag/offset", and that's quite speaking, without a chance of collision. Well, it would collide, if there were more than one pagination - but that in itself would be wrong on so many levels that I do not expect it to happen.

• mixed parameters: could you explain why exactly the parameters of the DataRetrieval used for view controls become mixed? Same for Data::withAdditionalParameters(). Do we really need to expect any kind of value there? Why don't we pass this data as an array anymore? If I understood correctly, these parameters should be somewhat similar to data retrieved by Form::getData(), no?

Actually, with transformations attached to the form(fields), there might be just anything in the parameters; "mixed" stresses this fact and allows for higher level objects than arrays in the future.

• Data::withAdditionalParameters(): can you explain what this method is used for briefly? Maybe a comment would help.

The original thought was to already construct the table with some environment vars the dataRetrieval might need, and just pass it around with "batteries included". It's just a way of attaching something to the table.
But you are right, a comment would help :)

Best regards,
Nils

[thibsy]

Hi @nhaagen,

Thanks a lot for your explanations.

Please note, before you take any action, we also reviewed the PR included here, which may cause some changes too. So it's probably best to make an iteration on #8350 first.

About the concrete changes, please answer the following questions:

• Table\Data::withAdditionalViewControl(): could you explain how the handling of this input works exactly? Maybe provide a dedicated example. I am not quite sure, what withAdditionalViewControlData() is used for. Why is this necessary? Maybe specify in a comment as well.
• Table\Data::withAdditionalViewControl(): how do we ensure to not accept duplicate view controls? I know that names will not collide, but it would still not be logical to accept a view control which already exists, right?
• Table\Data::getViewControls(): is there a specific reason why you are setting the table ID as dedicated name?

Please implement the following changes:

• mixed type hints: I understand the logic behind this; however, this PR is quite intrusive while the actual benefit of it is low. Maybe we should put some more thought into this and apply this change to all of the input-related things to a latter point. Therefore, please revert this change and add the $additional_viewcontrol_data parameter in the DataRetrieval as last one and make it optional (default value).
• View control names: please document the behaviour of the applied names somewhere, maybe the Input\Factory. Please also add a usage rule, describing how each individual view control should only be used once for its context.
• Input\ViewControl\Factory: please set the dedicated names inside the constructor of view controls, instead of the factory. There are already some duplicates now.
• Container\ViewControl\Factory: please use the FormInputNameSource when creating view control containers. Otherwise, we will run into naming collisions when they are provided with dedicated names. Maybe rename ViewControlNameSource to ViewControlInputNameSource for clarity.
• Conflicts: please rebase onto the latest trunk to resolve conflicts. Make sure the fix of Fix offset not in array if on last page and filter is applied. #8829 is properly applied here too.

Kind regards,

@Amstutz and @thibsy

[klees]

Hi @thibsy and @Amstutz,

regarding "mixed type hints": How strong is your resentment here? We have made the change for two reasons:

• The new parameter order seems to be more natural regarding the position of the various controls in the table.
• ?array is actually wrong, at least for the filter, since it can not respect any transformations that are attached to the filter.

Then, the best moment to change an interface often is "now", since the usages will only increase from now on. The second reason even is a borderline bugfix. So, can we seize the day?

Kind regards!

[thibsy]

We (UI coordinators and @nhaagen) discussed the DataRetrieval interface change and agreed that it will actually prevent errors in the future, because the return type of Input\Container::getData() already is mixed. This is due to Refinery\Transformation which could be applied to the container itself, making getData() return something other than an array.

@nhaagen we have just merged #8350, could you rebase this onto the latest trunk and check if #8829 is properly applied? I was referencing the wrong PR in my last comment. Thx!

[nhaagen]

@nhaagen` we have just merged #8350, could you rebase this onto the latest trunk and check if #8829 is properly applied? I was referencing the wrong PR in my last comment. Thx!

I was wondering already - but, yes, they are both applied :)

[thibsy]

@nhaagen is this ready for another round? If so, please also try and answer our questions.