@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

@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...

@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.

@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.

@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?

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

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.

@smg6511 for cli command, it'd be git remote add bxr https://github.com/theboxer/revolution.git where bxr is name of the remote, so you'll be able to do something like git pull bxr 3.x-issue-14961

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.

Ok, I've pulled down your changes and am giving them a run-through. I'd referenced the incorrect format above when making the case for using Immutable::createFromFormat it wasn't Y-m-d [yyyy-mm-dd, that's ok], it's a format like m-d-Y that fails; you'll see this when changing/saving a User's birth date. For that reason, I think that usage should stay. Of course we don't have to pull the date format from the Formatter as I originally did and could just getOption('manager_date_format', [default format]); note that the logic behind that initial choice was that, as it stands, users can delete that setting and I don't believe there's a system-wide default persisted anywhere. But we could resolve that in another PR that's more centered around how modx-installed data and settings are persisted and/or protected.

Anyway, I'll be back with any other comments in the next day or so...

@theboxer @opengeek @rthrash - (re John's updated version) Another thing I noticed is the use of typed properties. I'd love to make use of this feature, but we'd need to bring up the required modx php version to 7.4. Currently we're stating 7.2.5. Is there movement toward upping the required php version for the next (or the 3.1) release?

@smg6511 Revo 3.x already requires 7.4 (https://github.com/modxcms/revolution/blob/3.x/composer.json#L52)

Ok, I've pulled down your changes and am giving them a run-through. I'd referenced the incorrect format above when making the case for using Immutable::createFromFormat it wasn't Y-m-d [yyyy-mm-dd, that's ok], it's a format like m-d-Y that fails; you'll see this when changing/saving a User's birth date. For that reason, I think that usage should stay. Of course we don't have to pull the date format from the Formatter as I originally did and could just getOption('manager_date_format', [default format]); note that the logic behind that initial choice was that, as it stands, users can delete that setting and I don't believe there's a system-wide default persisted anywhere. But we could resolve that in another PR that's more centered around how modx-installed data and settings are persisted and/or protected.

Like I said, I have no issues using the Immutable::createFromFormat in the user's save/update processors. I simply stated that it shouldn't use methods/properties from a class that's called formatter, because at that point you'd be misusing class intended for formatting (outputting data for user) for input validation. I simply reverted that part, to the original and if you'd want to go ahead with the immutable update, I'd put it as a standalone PR, because it solves different problem than the current PR.

@theboxer - Ok, no problem putting the use of Immutable::createFromFormat in another PR.

BTW, I think everything's looking great otherwise. I have a couple small additions/tweaks based on your updates which I have incorporated on a new branch:

• Apply new formatter to Context settings
• Add/adjust method documentation in new formatter

On the php version, the modx.com/download page will need to be updated—it still claims 7.2.5 is the required version; the same goes for the Docs. Also, the phpcs.xml file needs updating as well (which I can do).

@smg6511 will you push all the stuff here?

Codecov Report

Attention: Patch coverage is 9.20097% with 375 lines in your changes missing coverage. Please review.

Project coverage is 21.53%. Comparing base (9a3f82c) to head (6150494).
Report is 8 commits behind head on 3.x.

@@             Coverage Diff              @@
##                3.x   #16604      +/-   ##
============================================
+ Coverage     21.47%   21.53%   +0.06%     
- Complexity    10652    10718      +66     
============================================
  Files           561      565       +4     
  Lines         32268    32521     +253     
============================================
+ Hits           6929     7005      +76     
- Misses        25339    25516     +177     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@theboxer - Ok, I think this is all set to do a final run-through. In the end, we just need to be sure that this does not end up in a 3.0.x backport, as the php version requirement is still too low there for the use of typed props (used in the Formatter).

There's one more think I want to mention, I notice in same cases we're switching from JS doing the formatting to the PHP doing it, this will result in different value getting displayed to the user

I don't think this should be a problem. All the core dates are stored as unix timestamps and users have control over the offset via the system setting. So long as we're consistently calculating and formatting dates on just one side of the equation (back end/php in our case), I think we should be fine.

If there's a worry here, I suppose the release notes could alert folks that, potentially, with this update datetimes displayed in the back end may be different than expected if a user's server_offset_time is not properly set.

If we have depended on the JS showing these in the proper timezone in the browser without needing server_offset_time to be managed before now, then this could be a problem.

@opengeek I did a quick run through of the changed js to be sure; I didn't see any issues when simulating different time zones. By and large the date formatting was being done in php before this PR—just in an inconsistent ways across the board—and we didn't change much in the js in that regard.

I'm fine with merging this one :)

This implementation still seems overly complicated to me. I don't understand the need for all these settings and a formatting class. PHP provides date formatting that should be sufficient to handle this.

@smg6511, can you elaborate on the necessity for the Formatter class? I.e. what does it do that can't be done natively with PHP? What else does it do? I'm not suggesting it's the wrong path. I'm just trying to understand it clearly and your intention.

@opengeek @jaygilmore - In a nutshell, the primary reasons for/advantages of the approach taken are:

1. First and foremost, any time I end up writing the same two or more lines to do the same thing in multiple places I look to abstract out that functionality. (I think MODX suffers noticeably on the code duplication front and I'd like to avoid adding to it.) Soon after digging into solving the issue this PR addresses, it became clear that the functionality should be consolidated in some way.
2. The date values taken in are not always the same type; sometimes they are a string value, other times (probably most times) a int timestamp. Add to that, strtotime can not handle certain formats (such as m-d-Y), which is why php's more capable DateTimeImmutable is used in the formatter class. Also, conditionals were needed in some places to account for various forms of an "empty" datetime value. This new service class provides a unified, one-liner way of formatting a date/time for string and int value types.
3. The two settings I did add give the user more control over display of full datetimes (the separator) and empty ones. IMO it's better to not hard code these things anyway; what if you decide for some reason you'd rather use a different separator as the default one? Currently you'd have to change several files to do so. You could argue it's a bit nitpicky, but I personally like something other than a blank for places (primarily the grids) that show and updated on value (and the object has not been updated yet).
4. And Jan's contribution of making this a service class (as well as simplifying my initial overly-heavy methods, breaking them up into smaller ones with more discrete purposes) might make this functionality more accessible for other uses.

All that's not to say it can't be improved further ;-)