Problem/Motivation

In OfficeHoursListItemFormatter->formatLabel(), "normal days" and "exception days" both use $settings['day_format'].

This is clearly unintended, since $settings['exceptions']['date_format'] exists.

Using $settings['day_format'] also produces chaotic effects. For instance, "3-letter weekday abbreviation" maps to the default date/time format "short" so it "works." However values like "two-letter" or "number" will either:

  1. work by coincidence if there's a text format by that name, like for "short"
  2. produce "some unreadable string" as seen in #3567869: Fix Exception day formatting when day format is 'number'

Steps to reproduce

  1. Set an Office Hours field's "Date format for exception day" to any value other than "short".
  2. Set the field's "Day notation" to "3-letter weekday abbreviation".
  3. On an entity with that field, add an exception that will show (per the display settings).
  4. Render the entity, and see that it ignores the chosen the date/time format.
  5. Set the field's "Day notation" to "number".
  6. Render the entity, and see that it uses the hardcoded "Y-m-d" format.
  7. Set the field's "Day notation" to "2-letter weekday abbreviation".
  8. Render the entity, and see that it produces a mess.

Proposed resolution

Honor the value, and remove code written to mitigate problems caused by the oversight.

Remaining tasks

  1. Use the "Date format for exception day" when appropriate.
  2. Remove the hack added in #3567869: Fix Exception day formatting when day format is 'number'

Issue fork office_hours-3576462

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mw4ll4c3 created an issue. See original summary.

mw4ll4c3 opened merge request !75

mw4ll4c3’s picture

mw4ll4c3 commented
Assigned: mw4ll4c3 » Unassigned
Status: Active » Needs review

This is a tested and working solution, as clean as it's going to get without reworking things. The test failures are inherited from the 8.x-1.x branch.

johnv’s picture

johnv
Primary language Dutch
commented
Assigned: Unassigned » johnv

Wow, you are active.
I am working on this issue.

  • johnv committed 507eefc8 on 8.x-1.x 
    Issue #3576462: Formatter ignores 'Date format for exception day'
johnv’s picture

johnv
Primary language Dutch
commented
Assigned: johnv » Unassigned
Status: Needs review » Fixed

Please check the new code.
I managed to remove the static code and use $item->label().

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

mw4ll4c3’s picture

mw4ll4c3 commented

I'm seeing the selected format on the latest 8.x-1.x -- thank you very much!

I can't vouch the Seasons stuff that's in there because I'm not using it (have to keep moving for now) but the little other little spot fixes are obviously fine.

And yes, I've tried to be active. Noticed it a while back, had no time, changed the underlying date/time format, came back later, found the root issue after #3567869: Fix Exception day formatting when day format is 'number', did a quick fix, and got that PHP error the day after (because I didn't clean up after myself).

My "fixes" had to be pretty shallow, but getting issues posted / bounced up was prudent, to air things and get them searchable... and offer "working-ish" stuff for whoever needs a small fix before the next stable release.

I'll keep doing that when I can, while you're cleaning stuff up in there.

johnv’s picture

johnv
Primary language Dutch
commented

Setting the satus back to Active, since an error is introduced:
- Formatter is incorrect when 'all day' checkmark is set.
- When adding (the first) exception, the date is set to '03-03-1973'.
- A second, third exception cannot be added

mw4ll4c3’s picture

mw4ll4c3 commented
Status: Fixed » Active

  • johnv committed 3b4d553a on 8.x-1.x 
    Issue #3576462: Fix error not showing in BaseSlot::...
johnv’s picture

johnv
Primary language Dutch
commented

I also tested how the Date formatter responds in formValidation in validateOfficeHoursSlot(). The errors did not show at all. Above commit fixes that.

  • johnv committed 8e60e973 on 8.x-1.x 
    Issue #3576462: Fix Formatter ignoring 'Date format for exception day'
johnv’s picture

johnv
Primary language Dutch
commented

Above commits fixes the following findings from #9:
- Formatter is incorrect when 'all day' checkmark is set.

Still to do:
- When adding (the first) exception, the date is set to '03-03-1973'.
- A second, third exception cannot be added

  • johnv committed d05a9411 on 8.x-1.x 
    Issue #3576462: Fix Regression for label of new exception item widget
johnv’s picture

johnv
Primary language Dutch
commented
Status: Active » Fixed

The reversal of setting NULL in first commit fixes the above regression problems.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

johnv’s picture

johnv
Primary language Dutch
commented
Status: Fixed » Needs work

There is a problem. The current/next css indicator is not set in some cases.

  • johnv committed ba69c3f7 on 8.x-1.x 
    Issue #3576462: Fis css when 'Replace weekday time slots with exception...
johnv’s picture

johnv
Primary language Dutch
commented
Status: Needs work » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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