Updated: Comment #24

Problem/Motivation

There is one special date format name (not currently a real date format) that should be reserved: custom.
If someone creates a date format with a reserved name, fancy things will happen.

Proposed resolution

In the date format form for creation/edit we should disallow this "special" strings.

This is implemented by adding custom to the pattern, (not the exists, because custom technically does not exist, so that would have been a lie).

this issue is *not* fixing that there are formats that are not shown in the ui.

Remaining tasks

None

User interface changes

None.

but here is an example error.

API changes

None.

Subtask of #2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed.
#2119903: Show locked date formats in the UI: Followup issue.

Files: 
CommentFileSizeAuthor
#24 dateformat-dot.png209.49 KBYesCT
#24 2100351-23a-disallow-custom-fallback-name-date-format-do-not-test.patch5.11 KBYesCT
#24 interdiff-21-23a.txt950 bytesYesCT
#24 2100351-23b-disallow-custom-fallback-name-date-format.patch4.87 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 59,449 pass(es). View
#24 interdiff-23a-23b.txt5.07 KBYesCT
#21 2100351-21-disallow-custom-fallback-name-date-format.patch5.05 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,443 pass(es). View
#21 interdiff.txt996 bytesGábor Hojtsy
#18 Screenshot_18_10_2013_14_00.png38.49 KBalexpott
#16 2100351-16-disallow-custom-fallback-name-date-format-test_only.patch3.17 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 59,253 pass(es), 1 fail(s), and 0 exception(s). View
#16 2100351-16-disallow-custom-fallback-name-date-format.patch5.09 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 59,531 pass(es). View
#16 interdiff.txt5.42 KBpfrenssen
#10 simpletest-inheritdoc-338403-80-82.interdiff.txt725 bytesgrisendo
#10 simpletest-inheritdoc-338403-82.patch4.24 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found. View
#7 2100351-disallow-custom-fallback-name-date-format-04-07.interdiff.txt1.12 KBgrisendo
#7 2100351-disallow-custom-fallback-name-date-format-07.patch4.48 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 59,421 pass(es). View
#5 2100351-disallow-custom-fallback-name-date-format-4.only-test.must-fail.patch3.01 KBgrisendo
FAILED: [[SimpleTest]]: [MySQL] 59,010 pass(es), 2 fail(s), and 0 exception(s). View
#5 2100351-disallow-custom-fallback-name-date-format-4.patch4.48 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 59,161 pass(es). View
#4 2100351-disallow-custom-fallback-name-date-format-4.only-test.must-fail.patch223 bytesgrisendo
PASSED: [[SimpleTest]]: [MySQL] 59,143 pass(es). View
#4 2100351-disallow-custom-fallback-name-date-format-4.patch4.48 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 59,295 pass(es). View
#1 2100351-disallow-custom-fallback-name-date-format-1.patch1.47 KBgrisendo
PASSED: [[SimpleTest]]: [MySQL] 59,144 pass(es). View

Comments

grisendo’s picture

Status: Active » Needs review
FileSize
1.47 KB
PASSED: [[SimpleTest]]: [MySQL] 59,144 pass(es). View

Patch attached.

grisendo’s picture

Assigned: Unassigned » grisendo

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.

grisendo’s picture

Issue tags: +Need tests
grisendo’s picture

FileSize
4.48 KB
PASSED: [[SimpleTest]]: [MySQL] 59,295 pass(es). View
223 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,143 pass(es). View

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

grisendo’s picture

FileSize
4.48 KB
PASSED: [[SimpleTest]]: [MySQL] 59,161 pass(es). View
3.01 KB
FAILED: [[SimpleTest]]: [MySQL] 59,010 pass(es), 2 fail(s), and 0 exception(s). View

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

penyaskito’s picture

Status: Needs review » Needs work

Awesome work!
Some naming stuff needs more work:

  1. +++ b/core/modules/system/lib/Drupal/system/Form/DateFormatFormBase.php
    @@ -112,6 +112,27 @@ public static function dateTimeLookup(array $form, array $form_state) {
    +  public function allowedMachineName($machine_name, array $element,  array $form_state) {
    

    This is checking for disallowed machine names, not for allowed machines names.
    Should be renamed to something like "isInvalidMachineName".

  2. +++ b/core/modules/system/lib/Drupal/system/Form/DateFormatFormBase.php
    @@ -129,7 +150,7 @@ public function form(array $form, array &$form_state) {
    +        'exists' => array($this, 'allowedMachineName'),
    

    The key in this array should be changed to something according to the new validation?

grisendo’s picture

Status: Needs work » Needs review
FileSize
4.48 KB
PASSED: [[SimpleTest]]: [MySQL] 59,421 pass(es). View
1.12 KB

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!

Status: Needs review » Needs work
Issue tags: -Usability, -regression, -Configuration system, -D8MI, -sprint, -language-config, -Need tests

The last submitted patch, 2100351-disallow-custom-fallback-name-date-format-07.patch, failed testing.

grisendo’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +regression, +Configuration system, +D8MI, +sprint, +language-config, +Need tests
grisendo’s picture

FileSize
4.24 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found. View
725 bytes

Let's try this one...

Status: Needs review » Needs work

The last submitted patch, simpletest-inheritdoc-338403-82.patch, failed testing.

grisendo’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, simpletest-inheritdoc-338403-82.patch, failed testing.

herom’s picture

Status: Needs work » Needs review

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

pfrenssen’s picture

Status: Needs work » Needs review
Issue tags: -Need tests
FileSize
5.42 KB
5.09 KB
PASSED: [[SimpleTest]]: [MySQL] 59,531 pass(es). View
3.17 KB
FAILED: [[SimpleTest]]: [MySQL] 59,253 pass(es), 1 fail(s), and 0 exception(s). View
  • 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.
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
38.49 KB

The problem is the error message makes no sense :(

Screenshot_18_10_2013_14_00.png

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

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Title: Disallow creating date formats with machine name custom or fallback » Disallow creating date formats with machine name 'custom'

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

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
996 bytes
5.05 KB
PASSED: [[SimpleTest]]: [MySQL] 59,443 pass(es). View

Made those changes. Should be back to RTBC then.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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',
      ),
    );
YesCT’s picture

I'm working on this.

YesCT’s picture

Assigned: grisendo » Unassigned
Status: Needs work » Needs review
FileSize
5.07 KB
4.87 KB
PASSED: [[SimpleTest]]: [MySQL] 59,449 pass(es). View
950 bytes
5.11 KB
209.49 KB

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:

dateformat-dot.png

Schnitzel’s picture

Status: Needs review » Reviewed & tested by the community

looks good! like this approach also much better :)

Gábor Hojtsy’s picture

Title: Disallow creating date formats with machine name 'custom' » Date format name 'custom' is reserved but not checked for
Category: task » bug
Issue tags: -D8MI, -sprint, -language-config

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.

ursula’s picture

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

ursula’s picture

Issue summary: View changes

Updated issue summary.

ursula’s picture

Issue summary: View changes

issue summary: removed remaining task

pfrenssen’s picture

Very nice solution, I like it :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed
+++ 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!

Gábor Hojtsy’s picture

Yay, thanks all!

Gábor Hojtsy’s picture

Issue summary: View changes

Issue summary update

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.