Hello guys.

I've been playing a bit with this module, and I think it could be useful to be able to define which rows are required. This is different to the global field property definition "required": If I check that option into the field settings, then all textarea/texfields will be required. But what about when we want only some specific rows?

For example, let's imagine following use case:

1. A team member "coach" starts a new "scoring node". He SHOULD define at least one row with "team names".
2. Then, he/she or someone else should fill team scores later. Removing a team is not allowed. This means, rest of rows are optional.

Patch on the way...

CommentFileSizeAuthor
#9 Edit Basic page Test Drupal standard.jpg36.31 KBlolandese
#7 tablefield-required_rows-3042239-7.patch7.67 KBwaspper
#7 tablefield_required-settings.png44.15 KBwaspper
#5 Test Drupal standard (1).jpg33.67 KBlolandese
#5 Table1 settings for Article Drupal standard.jpg96.66 KBlolandese
#2 tablefield-required_rows-3042239-2.patch3.34 KBwaspper

Comments

waspper created an issue. See original summary.

waspper’s picture

waspper commented
StatusFileSize
new tablefield-required_rows-3042239-2.patch3.34 KB

Ok, first attempt.

waspper’s picture

waspper commented
Status: Active » Needs review
lolandese’s picture

lolandese commented

Seems a valid and common use case. I will try to review as soon as reasonably possible.

Thanks for the patch and clear description.

lolandese’s picture

lolandese commented
StatusFileSize
new Table1 settings for Article Drupal standard.jpg96.66 KB
new Test Drupal standard (1).jpg33.67 KB

First review through:
https://simplytest.me/project/tablefield/8.x-2.x?patch[]=https://www.dru...

When defining a new tablefield the first row is already required for the "Default value", likely due to the fact that it is indexed as zero and the still empty value for $this->getSetting('required_rows') is considered as zero. Use of an isset() can probably solve that. Right now it would not pass an empty default value.

Screenshot

Some other points:

  • How to deal with multi-value tables that have different requirements for each table? See below the screenshot of an example.
  • What about if one would like a column to be required if a row is not empty? That seems a more common use case to me. See the first table of the screenshot below.
  • An upgrade path for existing table fields seems to be missing.

Screenshot

It is a good start for an awesome feature. It seems though that if you require specific rows and columns, you would also provide some default values. It might be good to add a checkbox in that UI instead where you indicate if a row or column is required.

Consider offering this functionality as a sub-module, as not all use cases would require this. The sub-module can then be enabled only if needed. Also, an extension module is an option with a dependency on Tablefield. Once RTBC I would agree with the sub-module approach as that keeps things neatly together in one package. The use case is still very common.

Thanks for the work so far. Meanwhile, anyone that has the same need can of course already apply the patch and start using it, considering the limitations described above.

lolandese’s picture

lolandese commented
Status: Needs review » Needs work
waspper’s picture

waspper commented
StatusFileSize
new tablefield_required-settings.png44.15 KB
new tablefield-required_rows-3042239-7.patch7.67 KB

Hello.

Sorry the delay for response. A bit busy :)

You're right. Some features I didn't consider at beginning. About your reviews:
1. I've moved field settings to the main field config form.
2. About multi-value fields: There is also a checkbox to inherit or not to the extra tables added. That way it's possible to define if only first table should have required fields or all of them.
3. Now it's possible to define indexes for columns or rows. Or doing a mixture of them.
4. At the end, I've opted for the sub-module. So, IMHO no need for an "upgrade path".

Here an screenshot to illustrate better goal of sub-module:
Tablefield required settings

So, let's check/validate approach, to do needed fixes/comments. Thanks for the initial reviews :)

waspper’s picture

waspper commented
Status: Needs work » Needs review
lolandese’s picture

lolandese commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Edit Basic page Test Drupal standard.jpg36.31 KB

Review through:
https://simplytest.me/project/tablefield/8.x-2.x?patch[]=https://www.dru...

The approach by adding a new piece of functionality through a sub-module is safer. I checked the impact on existing field by enabling the "required" sub-module. That is fine but one should be made aware that the validation takes place on form save of the entity. That means that existing tables might not have the required rows or columns until edited. This should be included in a README.md (no blocker).

Also the other way around, disabling "required" sub-module, is not impacting the content of existing table fields.

Personally, I would add the option to ignore empty rows or columns for the "required" check. It is pretty common to have a column, let's say age, that should contain a value only if a name is given.

Screenshot

This is not a blocker. I will leave this issue in RTBC for a week to see if there are any objections and will commit happily after that.

lolandese’s picture

lolandese commented

  • lolandese committed 1c1be14 on 8.x-2.x authored by waspper 
    Issue #3042239 by waspper, lolandese: Specific rows required
lolandese’s picture

lolandese commented
Status: Reviewed & tested by the community » Fixed

Committed. I opened a feature request on this functionality:
#3051260: Add the option to ignore empty rows or columns for the "required" check

Thanks for your contribution.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.