Skip to content

Decouple time widgets#350

Open
Jan-Schuppik wants to merge 40 commits intomainfrom
decouple-time-widgets
Open

Decouple time widgets#350
Jan-Schuppik wants to merge 40 commits intomainfrom
decouple-time-widgets

Conversation

@Jan-Schuppik
Copy link

@Jan-Schuppik Jan-Schuppik commented Jan 8, 2026

This PR removes the direct dependency of the time widgets on Icinga Web 2 (\Icinga\Date\DateFormatter::formatDateTime()) and introduces a native relative-time formatting behavior in IPL Web.

Resolve:

Depends on:

Create a general Time Widget which is also the base for
TimeAgo, TimeSince & TimeUntil
@cla-bot cla-bot bot added the cla/signed label Jan 8, 2026

protected $tag = 'time';

public function __construct(int|float|DateTimeInterface $time)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add PHPDoc.

Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't look at JS yet.

Also, add tests for Time and its child classes. Test formatting and rendering.

$values = ['at %s', 'An event happened at the given time'];
}

return sprintf(t(...$values), $time);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$values = ['since %s', 'A status is lasting since the given time, date or date and time'];
}

return sprintf(t(...$values), $time);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$values = ['at %s', 'An event will happen at the given time'];
}

return sprintf(t(...$values), $time);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clean up unnecessary methods
only allow \DateTime to construct widgets
simplify formating of time strings
And fix linting error in Time.php
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still no tests!

/** @var string Tag of element. */
protected $tag = 'time';

public function __construct(DateTime $time)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still no phpdoc for the constructor… is there a reason why you neglect to add this?

/** @var string Tag of element. */
protected $tag = 'time';

public function __construct(DateTime $time)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid, but I just noticed that requiring DateTime is a breaking change. At least as long as this is the constructor for the child classes as well.

To solve this, please override the constructor in the child classes and widen the allowed types so that it's possible to pass what was previously allowed, but cast it to DateTime before calling the parent constructor.

*
* Returns an array with the interval, the formatted string and the type of difference
* Type can be one of the constants RELATIVE, TIME, DATE, DATETIME
* Passing null as a parameter will use the current time
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it fails

[$time, $type] = $this->diff($this->dateTime);
$format = $map[$type] ?? $onMessage;

return sprintf(t(N_($format[0]), N_($format[1])), $time);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you didn't understand how t() and N_() work yet. Look at their implementation, look where they're used (especially N_) and read up what gettext is and how it works.

Then open a PR at https://github.com/Icinga/developer-guidelines-drafts/pulls to explain what you've learned so that others don't make the same mistakes.

Yes, I mean this serious!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a great idea, now i understand how they work!

We want to use DateTime objects for the Time class to work with
but the subclasses need to be able to handle also timestamps
Passing a timezone to `new DateTime()` when the input is a Unix
timestamp prefixed with `@` is silently ignored by PHP — the timezone
is always set to UTC in that case.

Fix by calling `setTimezone()` explicitly after construction.
Instead of stripping the sign with Math.abs() before rendering and
handling negation inconsistently across different calls, pass the raw signed
value through and derive the display sign in render()
The until -> ago transition relies on a cached TimeAgo format. If none
is cached yet, the transition silently fails.

Fix by pre-assigning the ago label to TimeUntil widgets when they are
animated (< 1h), ensuring the format is available when the transition
occurs.
No code path in Time::diff() ever returns the DATETIME type,
making the constant and its references in the subclass format
maps dead code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants