Date format name 'custom' is reserved but not checked for

Patch attached.

Working on test.
Maybe it should be good to specify somewhere that 'custom' and 'fallback' are not allowed, and try to change error messages on that cases.
But not sure where to inform (#description ?), and which messages to show.

Tests added. I think interdiff is not needed as here since only test is added, no other changes.

Oops, bad file for "only-test.must-fail" patch. Here are the good ones.

1. Oops, you are right :) bad naming there.
2. Don't know exactly if I understand. Word 'exists' is a reserved "parameter" for '#machine_name' field type. Nothing to do here I think.

Thanks!

#7: 2100351-disallow-custom-fallback-name-date-format-07.patch queued for re-testing.

Let's try this one...

Oops, wrong patch in #10, not for here... too much sprinting :(

This should be "needs review". and the patch to review is at #7.

Reviewed #7. Not bad, not bad :) :)

1. +++ b/core/modules/system/lib/Drupal/system/Form/DateFormatFormBase.php
   @@ -112,6 +112,27 @@ public static function dateTimeLookup(array $form, array $form_state) {
   +   * Callback to check if a machine name cannot be used for a new date format.
   

   "Checks if a machine...."

2. +++ b/core/modules/system/lib/Drupal/system/Tests/System/DateFormatsMachineNameTest.php
   @@ -0,0 +1,85 @@
   +      'name' => 'Date Formats machine names',
   +      'description' => 'Test DateTime date formats machine names.',
   

   The wording here is not very nice IMHO. Eg. "Date formats" in the name IMHO because this is not a module name or some other concrete thing. No good suggestion for the description, but it does not feel 100% right either.

3. +++ b/core/modules/system/lib/Drupal/system/Tests/System/DateFormatsMachineNameTest.php
   @@ -0,0 +1,85 @@
   +    $this->assertText('The machine-readable name is already in use. It must be unique.');
   ...
   +    $this->assertText('The machine-readable name is already in use. It must be unique.');
   ...
   +    $this->assertText('Custom date format added.');
   ...
   +    $this->assertText('The machine-readable name is already in use. It must be unique.');
   

   These asserts should assert the text with t().

4. +++ b/core/modules/system/lib/Drupal/system/Tests/System/DateFormatsMachineNameTest.php
   @@ -0,0 +1,85 @@
   +    do {
   +      $id = Unicode::strtolower($this->randomName(16));
   +    } while ($id == 'custom' || $id == 'fallback');
   

   This looks a bit strange. I see it is supposed to be bulletproof but a 16 long random string will *never* equal 'custom' or 'fallback', neither of which are 16 characters long. So no need for the do/while.

• Addressed remarks from #15. For the name of the test I was pondering "Date format machine names" or "Date format ids" and decided on the latter because it is shorter.
• Cleaned up namespaces list in DateFormatFormBase.
• Fixed some documentation and coding standards issues.
• Replaced the code in isInvalidMachineName() with a simple one liner.
• Removed the enabling of the System module in the test, this is not necessary.
• Provided assert messages.

Looks much better. RTBC if green as far as I'm concerned.

The problem is the error message makes no sense :(

Since when I go to the list of data formats I don't see a fallback or custom date format listed.

If so that is an existing problem:

$ls core/modules/system/config/system.date_format.*
core/modules/system/config/system.date_format.fallback.yml
core/modules/system/config/system.date_format.html_date.yml
core/modules/system/config/system.date_format.html_datetime.yml
core/modules/system/config/system.date_format.html_month.yml
core/modules/system/config/system.date_format.html_time.yml
core/modules/system/config/system.date_format.html_week.yml
core/modules/system/config/system.date_format.html_year.yml
core/modules/system/config/system.date_format.html_yearless_date.yml
core/modules/system/config/system.date_format.long.yml
core/modules/system/config/system.date_format.medium.yml
core/modules/system/config/system.date_format.short.yml

You don't see most of these on the page either (including fallback). So the same happens before/after this patch. In fact if this patch needs work, that sounds like it would be because fallback is already an existing entity, so we don't need to special case it, we only need to special case custom.

The ones that don't show up on this screen are locked and not editable. They do exist anyway.

So retitling since 'custom' is the only one that needs special casing.

Made those changes. Should be back to RTBC then.

Unfortunately system.date_format.custom.yml still actually does not exist though so the error message is misleading.

So reading the form_process_machine_name() code I don't think we should be futzing with the exists function. I think we should be providing a custom replace pattern that will produce a custom error message that informs the user that custom is a reserved word.

So

    $form['id'] = array(
      '#type' => 'machine_name',
      '#description' => t('A unique machine-readable name. Can only contain lowercase letters, numbers, and underscores.'),
      '#disabled' => !$this->entity->isNew(),
      '#default_value' => $this->entity->id(),
      '#machine_name' => array(
        'exists' => array($this, 'exists'),
      ),
    );

becomes something like

    $form['id'] = array(
      '#type' => 'machine_name',
      '#description' => t('A unique machine-readable name. Can only contain lowercase letters, numbers, and underscores.'),
      '#disabled' => !$this->entity->isNew(),
      '#default_value' => $this->entity->id(),
      '#machine_name' => array(
        'exists' => array($this, 'exists'),
        'replace_pattern' =>'([^a-z0-9_]+)|(^custom$)',
        'error' => 'A unique machine-readable name. Can only contain lowercase letters, numbers, and underscores. Additionally a date format can not use the reserved word custom',
      ),
    );

I'm working on this.

first interdiff to 23a is just docs cleanup for #338403: Use {@inheritdoc} on all class methods (including tests)

second interdiff does what @alexpott asked for in #22
and takes out an unrelated change reordering the use statements.

adds a test for invalid pattern, since now we added that to date format machine names we need to test it specifically for date format.

only patch b needs to be run on the testbot.

example:

looks good! like this approach also much better :)

This is not related to solving anything for the language system it is just a bug found on the way, so removing the D8MI tags.

Opened a follow up issue proposing to display reserved machine names and formats on the UI:
#2119903: Adding date format returns error for format not in the date-time list

Updated issue summary.

issue summary: removed remaining task

Very nice solution, I like it :)

+++ b/core/modules/system/lib/Drupal/system/Tests/System/DateFormatsMachineNameTest.php
@@ -0,0 +1,93 @@
+class DateFormatsMachineNameTest extends WebTestBase {

The only thing that seemed a bit weird to me was that these awesome tests are specific to date formats. Seems like this is actually testing the underlying machine name code moreso than date formats specifically.

However, that is something we could always clean up in a follow-up, and this patch looks straight-forward enough to fix this particular issue. Great work!

Committed and pushed to 8.x. Thanks!

Yay, thanks all!

Issue summary update