Show locked date formats in the UI

clarifying title

Issue summary update - formatting

I believe I can help here and am working on this.

The attached patch implements the proposed solution of displaying the locked date format fields in the date format entity list page. Similar to how the field ui displays locked fields by adding " (Locked)" in the label, the date format list does the same thing.

The below highlights the new rows. The path in the screenshot is admin/config/regional/date-time

cool. that is a great start.

@evilehk did we open an issue also to *allow* creating a new format even if it duplicates another patter? (not duplicate machine name, but pattern) I dont think we did, so

next steps:
* ui review
* open another issue to allow duplicate formats (and link it in the summaries of these related issues)

I think we need a usability review.
We can help that by looking around in core for other UIs that show locked things like this.

HM. the language detection and selection hides a type. it is invisible.
and we hide the not applicable, etc system language, often.
hm.
roles is kind of similar, in that it shows system things (but different in that we can change stuff about the roles, like their human names I think)

this might be a wont fix.

new idea:
put the system ones is a collapsed set
maybe with a label like "System date formats"

and, (optional) since there is no edit on the system ones, we might want to include the info we see on edit of the regular ones, like the php pattern.

In summary, we really need that other issue that allows creating duplicate formats.
Maybe with a little helper step that says: "hi, you already have these other formats with that same pattern. Do you really want another one?" ... well that might not be a great pattern. but we can discuss that in the other issue.

added related issue

Filter formats make the locked filter undelete-able. But it is still editable.

updated remaining tasks

Adding related issue #2120813: Allow several date formats with identical date format strings

Unassigning for no particular reason other than I didn't realize this patch was still assigned to me and do not want to discourage others from reviewing

Installed this patch on a clean install of D8-dev today. The patch applied cleanly and, the UI shows all the locked system date formats. The US is not very friendly at this point, new formats are added under all the locked system formats. (see screen shot)

There is another issue about removing the machine names from the UI list, Change UI to remove display machine name for date formats, and replacing it as a hover. If we do show all these system elements, does it really make sense to show them if we don't show the machine name except as a hover?

Why cant one edit these?

Because they are an enforced standard: http://www.w3.org/TR/html5-author/the-time-element.html#attr-time-datetime

#2091397: Create hook_help for Datetime module blocked by this issue. raising this as critical as well.

No, #2091397: Create hook_help for Datetime module is not actually blocked by this. The hook_help() should describe what is happening.
If we choose to change things in this issue, this issue will update the hook_help() accordingly.

This patch has:

1. Reroll of patch in #3
2. Addressing the sort issue reported by @wylbur at #8
3. Manual test (before vs after) for sorting fix.

The before and after screenshots show a different number of things...

We still need a before screenshot before any patch is applied.

Is the sort based on locked first (to put them at the bottom) and then Name (or machine name)?

fixing html

Thanks for the review @YesCT.

We still need a before screenshot before any patch is applied.

- added new set.

Is the sort based on locked first (to put them at the bottom) and then Name (or machine name)?

- It wasn't order by name in #14, but it does sort by name, if the lock status are same.

I don't think that "before" screenshot in #17 is really "before any patch is applied", unless the title and issue summary are way out of date. It doesn't seem to be illustrating the bug.

Really very sorry for making this more complicated :(

@jhodgdon is correct. The before is "before I fix the order problem" instead of "before *this* issue patch as a whole". Here is proper before/after.

Thanks!!

1. +++ b/core/modules/system/lib/Drupal/system/DateFormatListBuilder.php
   @@ -59,9 +60,9 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
      public function load() {
   -    return array_filter(parent::load(), function ($entity) {
   -      return !$entity->isLocked();
   -    });
   +    $entities = parent::load();
   +    uasort($entities, 'static::sort');
   +    return $entities;
      }
   

   You can remove this method override completely. ConfigEntityListBuilder::load() already calls the sort method on the entity class.

2. +++ b/core/modules/system/lib/Drupal/system/DateFormatListBuilder.php
   @@ -78,10 +79,28 @@ public function buildHeader() {
   +      $row['id'] =  t('@entity_id (Locked)', array('@entity_id' => $entity->id()));
   

   Use $this->t() please.

3. +++ b/core/modules/system/lib/Drupal/system/DateFormatListBuilder.php
   @@ -78,10 +79,28 @@ public function buildHeader() {
   +  public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b) {
   +      if ($a->isLocked() == $b->isLocked()) {
   +        $a_label = $a->label();
   +        $b_label = $b->label();
   +        return strnatcasecmp($a_label, $b_label);
   +      }
   +      return $a->isLocked() ? 1 : -1;
   +  }
   

   This should be moved to the DateFormat class directly. Also, the indentation is wrong.

I updated the title to reflect the solution, and date formats are owned by system module, unrelated to the optional datetime.module.

Thanks for the review @tim.plunkett

#22.1 - Fixed
#22.2 - Fixed
#22.3 - Fixed

Still the result is same.

Looks great, thanks!

Unless mine eyes deceive me, we're lacking explicit test coverage for this, so needs work for that.

Also needs work because the only other place I've seen this "(locked)" pattern is on the Roles page, and there it's "anonymous user (locked)," (lowercase L, emphasized) so we should do the same here.

Don't want to scope creep this too much, but off-hand, it feels weird to have more locked things in an admin than unlocked things that you can actually do something with, and having those two things co-mingling in the same list. Would it be easy/desirable to simply split the table in two (Custom / Locked), similar to Enabled / Disabled in the Views UI listing)?

If that's a huge ball of wax, feel free to just fix the first two points, though, unless someone from the UX team feels strongly.

updated issue summary remaining tasks, added contributor task document links, and tagged.

I want to learn how to write automated tests: assigning to myself.

Re-rolled patch because it didn't apply. Haven't made any changes yet, so no interdiff.

Changed "(Locked)" to "(locked)".

As an aside, I didn't notice any "locked" text on the Roles page currently, but this could be because of a recent patch.

Still have to write tests.

Added tests.

Feedback welcome.

Special thanks to @lauriii for mentoring me!

Thanks @mparker17 for the patch. Let's add an unique tag for date issues (as this is system.module component), so that we can track them.

reg #25:

Thanks @webchick for the valuable comments & QA.

we're lacking explicit test coverage for this, so needs work for that.

- @mparker17 addressed this in current patch

Also needs work because the only other place I've seen this "(locked)" pattern is on the Roles page, and there it's "anonymous user (locked)," (lowercase L, emphasized) so we should do the same here.

- @mparker17 addressed this current patch

Don't want to scope creep this too much, but off-hand, it feels weird to have more locked things in an admin than unlocked things that you can actually do something with, and having those two things co-mingling in the same list. Would it be easy/desirable to simply split the table in two (Custom / Locked), similar to Enabled / Disabled in the Views UI listing)?

If that's a huge ball of wax, feel free to just fix the first two points, though, unless someone from the UX team feels strongly.

Totally agreed on unifying UI part, but may be not this issue :). Let's fix both pages (and if there any other pages with this case) at #2280055: Unify the UI of locked+unlocked components list

Yes, I agree that all the points from webchick's "needs work" in #25 seem to be addressed. Back to RTBC. Thanks mparker17!

#2091397: Create hook_help for Datetime module is also (presumably briefly) postponed on this issue.

Committed/pushed to 8.x, thanks!