Problem/Motivation

TelephoneItem declares default_formatter = "basic_string". However, only string (and telephone_link) but not basic_string is available as a formatter for telephone fields.

Screenshot of the manage display screen with a telephone field, and inspecting the formatter select in Firefox

Proposed resolution

Fix the default formatter to be string.

Note that we could also change it to telephone_link. It's arguable which choice would be more backwards-compatible: Right now basic_string does actually get used when rendering by default (see FormatterPluginManager::getInstance()) and code string is functionally equivalent to that by default (i.e. when the link_to_entity setting is set to FALSE). However, when visiting the Manage display form, the telephone_link formatter is selected by default, because it is the first one. So just visiting that page and saving it without any (explicit) changes will configure telephone_link to be used.

Alternatively we could also make basic_string availabe for telephone fields. But both string and basic_string have the same label ("Plain text"), so we would have to fix that, as well, in order to not make the UI confusing.

CommentFileSizeAuthor
#10 3130240-10-test-only.patch5.77 KBravi.shankar
#7 interdiff_2-7.txt2.35 KBravi.shankar
#7 3130240-7.patch7.46 KBravi.shankar
#2 3130240-2-test-expansion.patch7.23 KBtstoeckler
#2 3130240-2-test-only.patch6.58 KBtstoeckler
#2 3130240-2-telephone-fix.patch661 byteststoeckler
Bildschirmfoto von 2020-04-23 11-43-19.png285.73 KBtstoeckler

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Active » Needs review
StatusFileSize
new 3130240-2-telephone-fix.patch661 bytes
new 3130240-2-test-only.patch6.58 KB
new 3130240-2-test-expansion.patch7.23 KB

Here's a patch with the proposed fix.

In thinking about test coverage I found FieldDefinitionIntegrityTest which I would have thought would catch this. It turns out that that only checks that the default formatter exists in general, not that it applies to that field type. So I think that would be the proper test coverage.

But, of course, expanding that test shows further failures: The datetime_timestamp formatter does not allow the changed field and the string formatter does not allow the uuid field. So also attaching a patch that fixes those issues, as those are fairly straightforward.

And also attaching a should-fail patch with the above but without the telephone item fix to validate the changes to the test coverage.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

I should note that I'm not sure if the issues reported in #2 should all be fixed here of if we should split this into multiple issues. I just wanted to (get and then) provide a full picture of the situation and provide a way to fix it as a whole. I do not in any way mind if committers prefer this is different steps.

The last submitted patch, 2: 3130240-2-test-only.patch, failed testing. View results

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

chi’s picture

chi commented
Title: Telephone field declares an invalid default formatter » Some field plugins declare an invalid default widget and formatter
Component: telephone.module » field system
Status: Needs review » Needs work

+ $this->pass()

This method has been deprecated.

I think fixing the other two similar bugs (#2) here makes sense as the also covered by provided test.

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 3130240-7.patch7.46 KB
new interdiff_2-7.txt2.35 KB

Here I have tried to fix comment #6.

chi’s picture

chi commented

@ravi.shankar Can you provide test only patch as well?

Status: Needs review » Needs work

The last submitted patch, 7: 3130240-7.patch, failed testing. View results

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 3130240-10-test-only.patch5.77 KB

Here I have added test only patch.

chi’s picture

chi commented

+ $this->assertTrue(sprintf('Field type %s uses an existing and applicable field widget by default.', $definition['id']));

That's wrong usage of assertTrue

Status: Needs review » Needs work

The last submitted patch, 10: 3130240-10-test-only.patch, failed testing. View results

chi’s picture

chi commented

I think the whole test needs to be refactored to replace conditional pass/fail statements with assertContains or assertArrayHasKey.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.