Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make date display consistently across manager components #16604

Open
wants to merge 8 commits into
base: 3.x
Choose a base branch
from

Conversation

smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Aug 25, 2024

What does it do?

  1. Adds a new Formatter class and updated several processor classes (and a couple controllers) to use the new date formatting methods provided.
  2. Adds a timestamp validation static method on the main modX class.
  3. Includes missing language topic for the Resource data page (currently two column headings are missing because the manager_log topic needs to be included).

Why is it needed?

The manager UI is inconsistent in its formatting of dates and times. This PR consolidates the formatting logic in one place and applies it to every component that displays a date (unless I've overlooked something).

How to test

  1. As this PR includes two new system settings, a data rebuild is necessary (/_build/transport.core.php) then a reinstall (upgrade) via /setup.
  2. Set a manager_date_format and manager_time_format, as well as a manager_datetime_separator in the system settings.
  3. Take a run through the entire interface and verify that areas that display a date/time do so as expected and match your expected format everywhere. Note that there are two places where these changes are purposely no applied: The system info page (where the server and local times are shown) and the manager error log (where it's impractical to try to alter the entry format).

Related issue(s)/PR(s)

Resolves #14961.
Resolves #16512.

@smg6511 smg6511 added the pr/review-needed Pull request requires review and testing. label Aug 25, 2024
Adds a new centralized formatting class for datetime data, adjusting various classes to make use of the new formatters.
Applied formatter to system settings processor and made a few other corrections to previously-committed code.
Should have included this with the last commit
Include empty date display value in early return
Applies formatter to this object type
(Finding areas I'd missed updating in earlier commits)
@theboxer
Copy link
Member

theboxer commented Sep 6, 2024

@smg6511 In general I like the proposed idea, but I didn't like the implementation much, it seemed a bit complicated.

Can you please check an updated version I pushed to my repo: theboxer@4133e16 and let me know your thoughts?

The key changes would be:

  • Renaming the formatter class to better reflect it's usage
  • Move the formatter under Formatter namespace, I can imagine we could have more formatters in the future
  • Removed the part where formatter was used as a parser in the edit profile and validate user (reverted to original)
  • Added the formatter into the DI container
  • Removed public $formatter from the generic processor, there's no need for it when it's not getting used on generic level
  • Simplified the formatter's functions, reduced number of arguments and converted them to separate functions, trying to keep SRP

@smg6511
Copy link
Collaborator Author

smg6511 commented Sep 6, 2024

@theboxer Thanks for going through it so thoroughly! I took an initial look and like what you did structure-wise. (I do sometimes try to do too much in some methods!) I do need to understand better how the use and implementation of services works. Anyway, let me pull this down and go over it more closely in the next few days or so.

The one thing I see that I will probably argue is your reversion to strtotime in some places. I chose to use the ...Immutable::createFromFormat because strtotime is incompatible with certain string formats (for example 2024-09-06 [yyyy-mm-dd], which I personally use a pretty often and I can't be the only one!) That said, I do need to closely check where you changed those to see whether it has the negative effect I think it might have.

Be back soon...

@theboxer
Copy link
Member

theboxer commented Sep 6, 2024

@smg6511

The one thing I see that I will probably argue is your reversion to strtotime in some places.

I'm fine with using the Immutable::createFromFormat, but I think it shouldn't be using anything from the formatter and probably shouldn't be part of this PR.

@rthrash rthrash added this to the v3.1.0 milestone Sep 12, 2024
@smg6511
Copy link
Collaborator Author

smg6511 commented Sep 20, 2024

@theboxer - Quick question: I've not pulled down a fork of a fork before; what's the best way of going about that? To this point I've used Github Desktop for all things modx git, but realize this may require some command line actions.

@opengeek
Copy link
Member

opengeek commented Sep 20, 2024

@theboxer - Quick question: I've not pulled down a fork of a fork before; what's the best way of going about that? To this point I've used Github Desktop for all things modx git, but realize this may require some command line actions.

@smg6511 — You should be able to add the fork as a new remote in your existing repository. I imagine GH Desktop has a feature for adding a remote?

@smg6511
Copy link
Collaborator Author

smg6511 commented Sep 20, 2024

Ok, so basically fork John's revo repo is what you're suggesting, right?

@opengeek
Copy link
Member

Ok, so basically fork John's revo repo is what you're suggesting, right?

No, you just add his fork as a remote to your local git repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recently edited resources crashes if createdon is 0 Display dates in manager in one format
4 participants