Allow exceptions to show for only today when "x days in future" is 0

Well, "later" came a lot sooner than I figured! Thanks so much!

I've got some other stuff to tackle, but I'll get back in double-check your "real" fixes soon (if still needed by then).

I did mean that. Sorry for any confusion.

Confirming: no PHP error with the latest commits to dev.

Indeed, it is not clearly defined how much 0 (or 1, 2) days in the future is. One might think '1 day in the future' means 'I can see tomorrow's exception.' So, 0 days means today.

That's how the code is treating it -- except for 0.

As of that latest commit:

• Two exceptions: today and tomorrow
• 0 days in future: no exceptions show
• 1 days in future: today and tomorrow show

While the whole thing could be changed from "days in future" to "days OF future" to keep 0 hiding things... that has its own semantic problems, and to allow "just today" we'd have to change the meaning of >= 1 as it exists.

I think a reliable solution and plan would be:

1. Allow today (and only today) to show when it's 0
2. Add a hard toggle for whether or not any exceptions show below the regular hours
3. Consider additional control (stuff for a different thread)

Thanks, velmir_taky. Nobody asks "why 37?" but it's a way better question. ;)

My question isn't "why 0?" It's "why not 0?" Two common answers:

1. because 0 means none
2. because 0 means all

Both ideas are supported by the code removed in MR77:

This wants to make 0 show them all...
2023-10-06: src/Plugin/Field/FieldType/OfficeHoursExceptionsItem.php:

if ($to == 0) {
  // All exceptions are OK.
  return TRUE;
}

...which never happens; because first this excludes them all...
2023-11-28: src/OfficeHoursItemListFormatter.php:

if ($horizon == 0) {
  // Exceptions settings are not set / submodule is disabled.
  return FALSE;
}

So, it (maybe) meant "all of them" for about two months, and now means "none."

"None" can be a toggle (or another formatter). "All" isn't supported at current but to cover it, I see two options:

1. raise EXCEPTION_HORIZON_MAX from 999 to 99999 (almost 300 years)
2. allow the input to be blank, rephrase the label to "limited to x days in the future"

The better option is "limit = blank = no limit = all" but that depends on what relies on it and where. The most I did on that was yank out #required and get a PHP error trying to save a blank value, naturally.

I'll be adjusting my MR:

1. roll my quick fix back
2. add those two removals from MR77
3. bump up EXCEPTION_HORIZON_MAX to 99999
4. add the variable and form to show/hide toggle
5. add a check to prevent them from printing below when hidden

I have this all built against yesterday's commit, but that breaks stuff, so I'll try to get something that works with 1.29 afterwards.

I opened #3577138: Allow exceptions to show for only today to handle all that from your latest commit.

The error's gone after that commit, so should we close this?

Thank you!

Unlimited "days in future"

Blank/null is definitely the way to go for "unlimited," so thank you.

Max horizon at 999 days

Now that "unlimited" is allowed, 3000-year max does seem excessive ;)

But a 3-year max could be a little tight for some orgs / purposes. Maybe give it one more digit? It seems harmless enough.

I almost removed the limit entirely in my MR, because why have it at all? But it keeps the size of the <input> reasonable, without getting into the CSS.

The checkbox

I think there's a misunderstanding; sorry.

I prefer the option "allow the input to be blank, rephrase the label to "limited to x days in the future"" above addint a checkbox

The checkbox prevents printing exceptions below the normal hours at all, ever, regardless of restrict_exceptions_to_num_days or anything else.

That way, someone can enter all the exceptions they want, for use in the "Replace timeslots" feature, and never have it print anything below.

This is now needed because an admin can't do that with 0 anymore.

On the next release, there would need to be an update, setting the checkbox to (restrict_exceptions_to_num_days !== 0), to keep things consistent for anyone who was using 0 to hide the exceptions.

The 'Replace timeslots' option works independent from the 'horizon' option. An internal list of the upcoming 14 dates is built, including the changes by exceptions and seasons, and injected in the 'regular' weekday formatter.

I was happy to notice that, but that's a different thing. The checkbox I added controls whether anything shows below the regular hours.

Turning it off does what used to happen with restrict_exceptions_to_num_days == 0. Suppose one of the 20k reported installs was doing that, because they could. Now they can't do it that way, and there's no other way to do it.

I've already argued against having one thing be impossible for the sake of allowing another. when there's a way around that. That goes both ways, so I'll stick up for it again :)

a simple use case

Someone wants to show the regular weekdays, using "replace timeslots", but not print the exceptions below.

That's it. That's all. Maybe it would mess with their design. Maybe they just find it redundant.

So they turn off the "show exceptions below" checkbox.

barely more creative one

Think "teaser" and "full content" display modes.

One biz/org, with multiple locations.
- The locations each have own regular hours, which can be different.
- Each location can (will) have different "exceptions" -- closures and changes, planned or unplanned.

There's one page that lists the locations in brief. "Teaser" stuff, y'know: address, a little text, and the regular weekday hours. Only what the visitor need at a glance.

Click on one of the locations, go to its node. "Full content" stuff: staff listing, maybe more text, and... the regular weekday hours with a list of upcoming exceptions. Stuff that's helpful to a visitor that's asked for more info, by clicking into the one location.

One data source, two presentations. One in brief, one with extended info.

Same field, two placements, two sets of field settings... and one checkbox.

The ultimate underlying reasoning is:

This is a module that fills a need, without a bunch of awkward wiring, has a nice client-friendly UI for input. It fills a gap.

With a few tweaks and options to fill its gaps, it can cover that need for about any conceivable case.

And yes, also a few inconceivable ones... maybe those 37 people are out there somewhere... ;)

"Same field, two placements, two sets of field settings... and one checkbox."
I do not think tht is true. All relevant settings are in the 'formatter' setting, which is unique for each disply mode."

You're right; sorry. I meant "two sets of that field's display settings" but you figured that out :)

Thank you for promising to work on it! Here are maybe my last thoughts on these two issues:

don't show exceptions below

I still think the right way is checkboxes for both "replace weekdays" and "show exceptions below"

The display settings offer two distinct, and optional, features: "printing exceptions within the weekdays" and "printing exceptions below the weekdays"

The checkbox means [do that thing at all]. The other settings then mean [how to do that thing] when [do that thing at all].

Clean and clear as it gets, for the user and the code: if that box isn't checked, everything else is ignored.

It's a common, flexible, and durable pattern for a reason: hard toggle for a feature, and then the feature's own settings. Frequently grouped, collapsible, hidden, disabled, etc. (I have of plenty of ideas about options for both features, but those are for other issues!)

A checkbox also skips any debate about the "limit to X days" setting...

allowing "no limit"

I think I saw a commit to make the max be 4-digit, so I agree with you: forget about it! :)

limit to X days

Now a solved problem, more or less. My only thought is allowing/using an empty number isn't great UX/UI.

We don't need to allow that, if we do one other thing...

final checkbox plea

Having no "limit" mean "no limit" was natural and intuitive.

Having no "limit" mean "don't do anything" would not be.

It would make its own sense... but only in the same way as if we made the exceptions date format optional, and no "exceptions date format" means "don't output the date" right? ;)

A toggle is simple, predictable, and independent of any decisions about the actual feature. Past, present, and future.

Please don't bring the limiter back into this, the poor thing! Not to give it a different edge case to control something else.

"Please don't use this one thing to control other things" covers like 95% of my concerns so far, even those other issues I'll post when I can :)

thanks again

I appreciate the work you've been putting in!

All of this is obviously just my opinion. Obviously I think I'm right, because it's my opinion XD

If you agree, there's code in #3577138: Allow exceptions to show for only today for the checkbox, which should still be good. The actual "don't print the exceptions below" is probably outdated, but it's the smallest part. (Obviously there's also days = 0 stuff too, but that was solved.)

And thank you again!

Thank you for staying right on top of this, and patiently hearing me out.

blank limit problems

Stuff like that is why I didn't bother when didn't "just work." More problems than it was worth, when we could just make MAX absurdly high.

how to enable/disable the display

-1 is better than blank, for the user and probably/maybe the code, but I'm still 100% "team checkbox"

They each mean one thing and one thing only. Very clear to the user. Nice and clean in the code.

Checkbox sits there, does its job -- its only job -- and does it well. One FALSE stops the whole feature in its tracks. number_of_days (or whatever it was) is always a number of days, with no exception. Even if it's negative (please don't ask!)

It's harder to break, less likely to cause conflict, and lower-impact on things that are being reworked. Very relevant right now.

updates!

Don't forget, whichever you decide, because we've changed what 0 means, so for existing display configs:

• set limit to -1
• set show_below (or whatever) to TRUE where limit > 0

For new display configs, I think FALSE should be the default. "More stuff" should be opt-in by default... that just seems Drupal-y. That's probably how I had it.

Ofcourse the is onlh 1 widget for 2 display modes :-(

Yeah :(

A "workaround" for that (and all of this) would be two fields, but nobody wants to hand that over to the client/end-user.

Rename 'x days in the future' to just 'x days', where 0 = no days (none), 1=1 day (only today), etc.

I wouldn't say no to that, at least for the time being. It covers both needs (today and nothing) and the input-to-output makes sense.

I prefer that to using -1, a lot, with all respect to the amount of working updating that MR must have taken.

Long-term, leaving room for changes and additions is why I'm still #teamcheckbox.

• The UI/UX of the "off/on, settings" pattern, will be helpful as settings get added
• Keeping the toggle "immune" to changes in other settings
• Freeing the settings from bearing that responsibility and having to cover them

My first (and so far best) idea involves three more options (across both features) and a change to this one... so I really like the idea of clean on/off switches that having nothing to do with any of that, y'know?

I don't even mind the idea of having the switch and making the other change to the label / behavior (because it makes that much sense).

Yeah, someone could turn it on but set it to show 0... but someone could also set up a View that only shows a node if its ID is two different numbers at the same time, so *shrug*