Allow end date to be optional

omkar.podey → made their first commit to this issue’s fork.

kunal.sachdev → made their first commit to this issue’s fork.

Looks good! Left a minor feedback. Also, this most probably requires a change record.

Addressed review and created CR.

Hiding files as fix is in MR.

Left some small comments.

Posted a review in the MR

Reviewing the remaining tasks and was only able to say that 1 is done.

Since this isn't using a formatter anymore first bullet may not be needed but could someone familar with the issue confirm?
Same for 2nd bullet

If not needed "Needs test" tag probably can be removed.

Yes, I think the 1st and 2nd bullets in remaining tasks are not needed and about the last bullet in remaining tasks I don't think we have to do it in this issue because the current behaviour in the MR is to show only start date when end date is missing which I think is fine for this issue.

Applied the MR on a Standard profile install of 11.x
Added a range field to the Basic page content type.
Checking "Make End date optional"

Created a Basic page and didn't set an end date. Node saved without issue
Went back to the settings unchecked "Make End date optional"
Went back to my Basic page and now I'm required.

Ran drush updb and post_update and install hook ran without issue.

How come the setting is under the field storage though and not a form formatter setting though? Won't this mean editors can't reuse this field in 2 places where 1 should have optional end date and the other not?

Also think issue summary could use some work.

add validation constraint

may be having a case of the Mondays but was a validation added?

Also there are some UI changes that are missing from the issue summary.

I am going to work on shifting the setting to field settings so that this field can be reused in 2 places where 1 should have optional end date and the other not. I am also going to work on adding the validation constraint.

Only if you think that is best approach. Wasn’t saying that it was needed more general observation/discussion

Added history of discussion to IS to enable easier reviewing of the discussion so far.

The key questions

5 key questions have recurred again and again over the life of this issue:

1. Is the start date optional too?
2. What value is stored in the end date property if it is left empty in the widget?
3. Does the optional_end_date setting live in the field config or field storage config?
4. How are optional and non-optional properties distinguished in the widget?
5. How are missing end dates displayed in the formatter?

I have added these to the IS. The answer in the current MR is:

1. Is the start date optional too? NO
2. What value is stored in the end date property if it is left empty in the widget? NOT SURE
3. Does the optional_end_date setting live in the field config or field storage config? STORAGE
4. How are optional and non-optional properties distinguished in the widget? NOT SURE
5. How are missing end dates displayed in the formatter? ONLY START DATE IS SHOWN

I think we need to be careful to keep track of our answers to these important questions in the IS, even as we work on the details of patches.

Kunal

@kunal.sachdev some feedback/requests for you:
A. Please could you tell us the answer for the questions I've answered "NOT SURE" above
B. In #48 a subsystem maintainer suggested to keep the setting in the field storage config
C. Please post a screenshot of the widget so we can see that the "required" indicators follow th suggestion made in #144

Tests

Lastly, I believe that even though we are not making large changes to the formatter and widget, we still need to explicitly test that they work with missing end dates. Therefore we need both formatter and widget tests.

Scope

If this issue is to ever get committed, realistically we need to keep the scope tight and defer to follow-ups anything we can. Therefore I suggest:
- Optional start date. This is a valid use case, but let's open a follow-up issue for it.
- Customising display of missing end date. This is a valid use case, but let's open a follow-up issue for it. Per product manager's usability review in #39, core should not offer complex options here and it is fine to just display start date if the end date is missing.

Therefore I think the current MR is correctly scoped.

Answering some of the questions asked in #180 ✨ Allow end date to be optional Needs work :

1. What value is stored in the end date property if it is left empty in the widget? NULL
2. How are optional and non-optional properties distinguished in the widget? I think asterix (*) is used to distinguish between optional and non-optional properties.
   =

In #48 ✨ Allow end date to be optional Needs work we have suggestion to keep the setting in the field storage config but I do agree with #174 ✨ Allow end date to be optional Needs work that if this setting is kept in the field storage config editors can't reuse this field in 2 places where 1 should have optional end date and the other not that's why I am going for shifting the setting to field settings.

Screenshot of the widget if the end date is optional:

Screenshot of the widget if the end date is required:

2. What value is stored in the end date property if it is left empty in the widget?

The answer can only be NULL, or to use a separate Boolean like end_date_is_all_day.

This issue happened before, in the Date module for 7.x. See #3202962: Expand field specification with new element for "all day" → . Using a "magic value", such as storing the date with a certain time to indicate that it's an all-day value, was a very bad approach that led to multiple issues. It eventually was rewritten in 7.x-3.x, but bugs linger in the 7.x-2.x branch and it's hard for sites to update or clean their data that was messed up.

#35 discusses some specific problems with putting the setting on the field config not the field storage config.

The screenshots in #181 match the consensus suggestion in #144.

For now I am keeping the setting in field storage config and I think we can decide on shifting the setting to field config in another follow-up.

Created follow-ups:

1. ✨ Allow start date to be optional Active
2. 📌 Customise display of missing end date. Active
3. 📌 Shift the optional_end_date setting from field storage config Active

@kunal.sachdev Thanks for creating the follow-ups. Can you please set the status of the follow-ups to Postponed and post a comment that they are postponed on this issue when you change the status? You can also add this issue as a parent issue or as a related issue. I don't think it makes sense for someone to work on these issues until this issue is fixed.

Also, I think it might be a good idea to list the follow-up issues somewhere in the issue summary, because this issue is pretty long and takes some time to read through, so a good issue summary that has all the relevant information is even more important for this issue to make it easy for core committers and other contributors new to the issue.

The issue summary also lists a remaining task (2.) that is not marked done yet. Can you please update the IS with the current approach in your implementation and/or with a reference to the follow-up you created, so that it is clear that the task is either done or out of scope?

W/r/t the last follow-up, a subsystem maintainer mentioned in #48 that the setting should be kept in field storage. Unless there is compelling evidence that the reasons given by them are maybe outdated in the meantime (they commented 5 years ago, so it might no longer be true due to some other changes in core), I think we should follow their advice and keep the setting in storage. If we think the setting should be in the field settings, I don't think we should have a follow-up, but I think we should do it in this issue. It doesn't make much sense to introduce a new setting now and then shift that from storage to field settings later. We could then use a field setting right from the start.

Closed 📌 Shift the optional_end_date setting from field storage config Active and added new remaining task to decide if we want to shift the optional_end_date from field storage config or not.

Thanks kunal. Added remaining tests needed to IS

may be having a case of the Mondays but was a validation added?

Yes the validation constraint is added in \Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem::getConstraints and test is also added for that \Drupal\Tests\datetime_range\Functional\EntityResource\EntityTest\EntityTestDateRangeTest. I think I am going to remove that constraint and add a separate constraint class and its validator because I think it's the more correct way to do it.

Discussed about some points with @lauriii and @tim.plunkett in scrum :

• Discussed about the validation constraint which is currently added in \Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem::getConstraints and decided to keep it that way.
• Discussed about the optional_end_date setting which currently lives in the field storage config and decided to keep it as it is for this issue and if we want to shift it to some other settings then we can do it in the follow up I opened 📌 Shift the optional_end_date setting from field storage config Active .

Just reading through the MR now in detail; it looks like the MR is not actually configuring whether the property is required or not in the schema level. It looks like there were some challenges with this approach in #33. I did find \Drupal\Core\Field\Plugin\Field\FieldType\StringItemBase::propertyDefinitions which configures the property based on the field definition, so I wonder if that's worth trying.

However, if we are not configuring this on the property level, but instead in the validation constraints, then this could be a field config setting, because it could be applied according to the field config. We have pre-existing examples of this like \Drupal\Core\Field\Plugin\Field\FieldType\NumericItemBase::getConstraints.

I do think @tim.plunkett raised a compelling argument that in some cases you may want this field to have different behavior in different bundles, which would support the idea of configuring this in the field config. So the question then becomes, is it acceptable for the end_value property to be always optional, for the requirement to be enforced in validation constraints depending on the field config setting?

Yes - I agree with @lauriii and @tim.plunkett, making this vary per field config rather than storage if there's no DB schema change feels like a good approach.

optional_end_date setting is moved to the field config and hence closed 📌 Shift the optional_end_date setting from field storage config Active .

So we have consensus on the solution, and the MR is complete.

I am not convinced a further usability review is needed, so I think this can be RTBC after code review of recent changes.

Looks good to me except some suggestions and one question.

@kunal.sachdev believe as the starter of the MR you can close the threads just FYI.

Nice to see it moved out of the storage setting.

Tested as before
Created a range field with "Optional" setting unchecked
Created a node and got an error about there being no end date.
Checked the "Optional" setting
Created a node without an end date and could save just fine.

Using a 2nd content type
Reused the field and set the setting to the opposite of the first content type
Settings were honored on both content types.

LGTM!

Nice work on this issue and moving out of storage settings.

1. This sentence is really awkward and fails readability IMO: "Allow end date to be optional opposed to the default behaviour where end date is required when "Required field" is checked."
2. I also think that field should be disabled unless the "Required" is checked. This setting makes no sense if the whole field is not required.

For the first issue, would this work better? Allow the end date to be optional, even if the date range is required.

For the second issue, that would be solved with #states api.

I haven't looked in the MR yet but the checkbox shouldn't have anything to do with the "Required" checkbox. The "Required" checkbox controls if the datetime range field must contain a value. The new checkbox added here, defines what is a valid value for the datetime range field; whether we expect to always have a pair of start and end, or can we accept just the start date. The new setting should impact the validation even when the field is not required. If the field is not marked as required, the end date should be still required when the user enters a start date.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

sime → closed merge request !5356

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2023-11-10 Active . That issue will have a link to a recording of the meeting. For the record, the attendees at the usability meeting were @AaronMcHale, @Emma Horrell, @benjifisher, @rkoller, @sime, and @simohell. If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

In the review we compared the setup of this option to the LinkItem options. The recommendation was that the checkbox "Optional end date" be converted to radios. This has two benefits

1. Radio options are easier to describe (breaks up the logic)
2. Makes a future change to add "optional start date" easier, as simply a third radio (see screenshot).

I have coded this change and created a MR against the existing MR, rather than just adding to the current MR. The functionality currently works as expected, so aside from changes to interface language, this is a positive review of the existing MR.
https://git.drupalcode.org/issue/drupal-2794481/-/merge_requests/2

I'm not confident that I have the language right, but I opted to be a little verbose. This isn't easy text due to the confusion that is caused in combination with the "required" option.

Thanks for adding the meeting summary @sime! There are two additional details to note that were mentioned on Friday.

1. If you add a field to a content type you have the field type Date and time and the dedicated field type Date range. Wouldn't it be more consistent to add Date range as an option to the Date and time field type? (in the context of the changes ✨ Make field selection less overwhelming by introducing groups Fixed introduced)

2. Out of the scope for this issue but the error message for the case if you leave the start date empty (the required field checkbox unchecked) and have end date required or not required. in both cases the user gets the error message This value should not be null. In particular for none technical users that message isn't helpful at all.

In regards of the microcopy for the radio button group introduced in MR5356. I wonder if the labels could be simplified? How about the following. Provide the context for the radio button group by changing its title from Optional values to simply End date, that way end date hasn't to be used within the list of available options? For the two available options instead of Not applicable, the end date is not optional simply use Required while changing Optional end date to just Optional? And i wonder if the description could be simplified as well to something like Applies to required and not required fields.?

but in general after testing MR536 going with the radio button pattern is indeed way more clear compared to the single checkbox pattern from my perspective.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

#210-1. I think i missed that part of the meeting. I think it's safe leaving this out of scope since date vs daterange have different storage needs, both occupy a lot of use-cases, and daterange is not visible if not installed (most people won't use it in favour of two date fields).

#210-2. Looking more closely at this, I think this is caused by changing the constraint validation which seems to upset the default HTML5 validation. It's arguably in scope and I'm reading back and I don't see anywhere that we made it out of scope.

#210-3. I really like this wording @rkoller! I did add a variation of the description where I think we can be more verbose Applies regardless of whether the whole field is required., but primarily the label and option labels are perfect. I have done a screenshot for those two options visual.

I have pushed a fix for "This value should not be null" and updated the widget option labels on my MR https://git.drupalcode.org/issue/drupal-2794481/-/merge_requests/2

Just confirming that my MR is against @kunal.sachdev's latest branch/MR as at #200.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The above CSpell error will be fixed if we merge my branch into the MR branch.

re #212 ✨ Allow end date to be optional Needs work

#210-1 yeah on second thought i completely agree, you are right. it is out of the scope for this issue and should go into a followup issue instead. the grouping has nothing to do what this issue is about.
but for the follow up issue from my perspective i strongly vote to sort the date range as an option under the Date and time group so all field types about date and time are contained there. those groups are user facing and are from my understanding communicating a common thematic functional topic. and even if a date field and a date range field have a different storage need i am not sure if it is necessary and beneficial to have two separate groups for them. i would keep the number of groups concise imho.

#210-3 one note regarding the optional-option. my suggestion was to use just Optional instead of Optional end date since "end date" is already used in the title now - therefore i would strike the end date in the option label.
About the description. I am torn, neither my own suggestion nor the variation you've proposed in #212 completely convinced me. The problem with my own is the "required and not required" is sort of hard to comprehend even though it is clear and is using the wording from the "required field" checkbox label. on the other hand "regardless of whether the whole" is sort of verbose and using the passive voice at the end (i am not sure how to express what it is but from a none native speakers perspective i am getting the core message the sentence communicates but it still makes me think each time i read it) . both variants are not perfect and make the reader think but i consider each variant still an improvement. i am fine with either of the two if no one else comes up with another variant.

#213 ✨ Allow end date to be optional Needs work i've tried to apply https://git.drupalcode.org/issue/drupal-2794481/-/merge_requests/2.diff with composer patches instead of MR5356 but it failed to apply.

@rkoller looks like @kunal.sachdev has merged (thanks @kunal.sachdev!), so no need to mess with my extra MR now, just use the normal diff.

I've created 📌 Date range should be in the date_time category Needs work for the grouping/sorting. It is easy to group it but it requires some UI text to be written.

I have changed t('Optional end date') to $this->t('Optional'), and pushed. Good change. So that leaves us with this description text which I agree might be improved.

at first thanks to @kunal.sachdev for the merge and the follow up work!

and thanks for opening up a follow-up issue for the grouping/sorting @sime as well as changing the label for the option! looks good! About the description. One detail i realized, why not simply strike the word "whole"? It isn't adding any mandatory information, and you aren't able to require a field in part anyway? I also searched if there is a way to express "regardless of whether" in a more plain way. The only variant i came up with was:

Applies no matter if the field is required.

but it reads a bit rough due to the use of "if". so not sure if there is a more seamless and easy reading alternative for "regardless of whether". but by striking the word whole your variant also looks better?

Applies regardless of whether the field is required.

I leave the final decision up to the english native speakers. ;)

> Applies regardless of whether the field is required.

I've pushed this change, I think it's looking pretty good at this point.

Thanks! I agree looks pretty good. One detail left to clarify in regards of the mentioned changes in #213 ✨ Allow end date to be optional Needs work . I've quickly retest with the current state of the MR ("required field" unchecked, end date option, and on the node only entered an end date and left the start date empty) and i still run into the This value should not be null. error message.

I agree that "Applies regardless of whether the field is required." sounds good.

Can confirm I got the same error as @rkoller in #221

Yeah same for me... not sure what changed there. I'm confident this needs (more) functional tests at least for optional fields. I plan to work on this.

While creating a date range field, one of the End date value (Required/Optional) should be selected by default.
Right now it is allowed to save without selecting any value.

#225 @narendraR - I looked at the LinkItem for comparison and note that it also sets initial defaults - so I think this is valid point and I've made this change. The initial default that I have chosen is to make the end date mandatory by default.

I've reverted my attempt to fix "This value should not be null" UX issue when the field is optional. It believe it would need to have the NotNull constraint (that is coming through via the TypedData I think, and not any parent classes) replaced completely. It's a bit beyond me to fix this. Since it has been considered out of scope previously I'll mark this Ready for reivew again.

If we are going to leave the message as is is there a follow up for fixing it?

Re:

I've reverted my attempt to fix "This value should not be null" UX

Agree

Code changes looks good to me. Tested manually and things are working as expected. Checked schema and config level changes also.

@smustgrave yes i have added a #4 TBA in the follow up issues, If no one objects i'll create that issue soon with my notes from investigating it.

I've made an issue for the UX issue.

Thanks for opening that.

Since that was the last item believe this may be good to go.

The current UI is confusing because we have the statement "Applies regardless of whether the field is required". This is not true. If the date range is not required and the end date is required then you are able to save a field without an end date. This is a good thing. But this text does not make it clear that this is the way this will be work. I think the ui needs refining to be something like this...

Under the hood this needs to be split out into the two settings but the UI should focus on user interactions not the underlying data model.

The current UI is confusing because we have the statement "Applies regardless of whether the field is required". This is not true. If the date range is not required and the end date is required then you are able to save a field without an end date. This is a good thing. But this text does not make it clear that this is the way this will be work.

Most of the difficulty around wording is because of the wording of the Required checkbox in the base/parent class. Do we want to explore unifying the two form elements (while retaining the underlying config structure)?

I also thought the current description adds more confusion than it helps. We have an existing pattern for this with the link field, that is just a separate element with hidden/optional/required.

I'm not sure about merging the two settings together and altering the form elements of the parent and if that's a good idea from a technical perspective.

The proposed solution IMHO is also not correct. Optional as a separate option isn't correct. Even if the field is not required, if a start date is given, you must provide an end date as well. So one more reason to not mix it. IMHO the description should be conditional, like "Whether an end date must be provided *if* a start date is given".

The description should be conditional, like "Whether an end date must be provided *if* a start date is given".

+1 for this suggestion.

This description is clear and easy to understand IMHO.

For full set of use case options I suppose we should have here:

Case a - when the "Required" field checkbox is not checked

1a. End date is required when the start date is provided (default behavior currently - selected as default for BC reasons)
2a. End date is optional when the start date is provided
(3a.) End date can be provided even if the start date is not provided

Case b - when the "Required" field checkbox is checked

1b. Both start date AND end date are required (default behavior currently - selected as default for BC reasons)
2b. Start date is required only
(3b.) End date is required only
(4b.) Start date OR end date is required

The radio options 3a, 3b and 4b are postponed for later addition. The follow-up issue is here: ✨ Allow start date to be optional Active

Note: English is not my native language, so the better wording is welcome of course.

+1 to #237. Let's move figuring out support for ✨ Allow start date to be optional Active to the follow-up. We'll have to change the UI once we introduce the more complex set of settings, but we don't have to do that here.

Im trying to attach that "diff": https://git.drupalcode.org/project/drupal/-/merge_requests/4619.diff on a Drupal 10.1.6 version, and It does not work. Could you review it please?

Grateful for your work. Thanks

Let's move figuring out support for #3395096: Allow start date to be optional to the follow-up. We'll have to change the UI once we introduce the more complex set of settings, but we don't have to do that here.

So revert it to a checkbox is my takeaway from this? I did restructure things to be able to handle optional start date without any impacts on the config schema.

Believe additional feedback has been addressed.

BTW thank goodness for gitlab, if this ticket were still comments and patches the ticket may not load haha.

lauriii/berdir/alexpott have outstanding concerns. Maybe it's pedantic but i want to address some concerns again.

It should work like LinkItem - structurally the setting is working like LinkItem, being a setting with enumerated options. But I think the comparison should stop there. IMO there is subtle difference between LinkItem "make the label optional" and DataRangeItem "make half of the data structure optional".

We made it more complex by considering future optional start date - agreed, but it helps to split the language up because you can describe each radio option separately, and the side effect is that it will save a future update hook if we add optional start date. If core devs want a boolean option, then I will re-roll the patch because I was the one who turned the checkbox into radios.

we shouldn't use "OPTIONAL_" language in the code - I don't think this should be a deal breaker. when i was looking at the code, i foudn that mentally differentiating between "required" (field) and "required" (part of a field) is mental gymnastics. So using the language "part of this required field is optional in this situation" I think is a solid step, and also matches how we describe it in the issue title.

The form labels are confusing - totally valid. rkoller and I considered different options and think it's pretty close. If we can settle on the underlying data model/code then we can finalise the labels?

Sounds like it should be back in review

I still think that having the required checkbox and separate radios for the end date is confusing. I think the concerns about unifying everything into a single set of radios expressed by @Berdir in #237 are incorrect. The UI does not have to match the underlying data structure.

This change gives us three states to deal with:

• end and start date required
• start date required
• nothing required.

If we do the follow-up to make it possible to just require the end date and not the start date then that adds "end date required" to this list and the it'll be simple to add the list of radio options.

I still think that having the required checkbox and separate radios for the end date is confusing.

. Should this go to the UX team?

Maybe it is a bit too late to radically change the concept here but I'll ask nevertheless...

Is it possible to have here two different sets of radios which are shown/hidden by JavaScript?

One set available if the "Required" checkbox is unchecked and the other set of radios available when the "Required" checkbox is checked. As suggested in comment #239.

That would give us enough flexibility for all options we need both currently and in the future.

Lets get this one back on the radar.

MR currently is unmergable.

@alexpott #247 what additional task should be added to the issue summary?

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

wow, this has been an issue for 7+ years?

Any reason not to just use https://www.drupal.org/project/optional_end_date →
?

@webengr If you go to that module, it even says on the main page that it's just a workaround until this is properly implemented in Core. I think we all want to see it in Core.

@alexpott in #247 believe the follow up is/should be covered in ✨ Allow start date to be optional Active .

Re-tested the MR

When using the End-date as required there is no visual indicator that it is required. Hopefully an easy tweak.

Functionality wise it was working as expected.

The challenge with #247 is that there's no such state as nothing required. When nothing is required, if you enter end date, the start date would still be required. I think the current proposal is good enough, and that's why I think we should move forward with it. We can implement improvements on top of this in future.

So fine with the accessibility issue of no required indicator?

no such state as nothing required

Yes there is - the field can be empty. If the required is unticked that you do not have to enter a start date or an end date.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Do this do anything that ✨ Add option to show only start or end date in the DateTime Range custom formatter Needs review doesn't do? If it does, I think it needs to be re-worked now that the other issue has committed.

Making #153 compatible with Drupal 10.3.