String field formatters cannot link to their parent entity

You need to take into account that $entity->urlInfo() can throw an exception. See #2355245: ER's label formatter needs to take into account that $entity->urlInfo() might throw an exception.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -27,7 +32,89 @@
   +        '#title' => $this->t('Link to the @entity_label', ['@entity_label' => $entity_type->getLabel()]),
   +        '#type' => 'checkbox',
   

   #type should be first?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -27,7 +32,89 @@
   +      $build['#markup'] = $this->t('Link to the @entity_label', ['@entity_label' => $entity_type->getLabel()]);
   

   Shouldn't this be "Links to ..."? We're not displaying an action here.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -38,7 +125,19 @@ public function viewElements(FieldItemListInterface $items) {
   +      if ($this->getSetting('entity_link') && ($entity = $items->getParent()->getValue()) && $entity->hasLinkTemplate('canonical')) {
   

   $entity = $items->getEntity().

4. +++ b/core/modules/simpletest/src/AssertContentTrait.php
   @@ -141,6 +141,16 @@ protected function parse() {
   +  protected function getUrl() {
   

   Where is this used?

This is a fabulous issue. Thanks so much for doing this. Only one comment.

+++ b/core/config/schema/core.entity.schema.yml
@@ -225,6 +225,13 @@ field.widget.settings.checkbox:
+    entity_link:

This is called link_to_entity in comment, etc. (and prior to that link_to_node, etc) in entity type specific cases. So using link_to_entity here would be consistent.

Awesome, @dawehner++

A couple nitpicks:

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -27,7 +32,89 @@
   +      $container->get('entity.manager')
   

   Yeah, only used for the settingsForm() / settingsSummary(), meaning 1% of actual runtime uses of the class.
   Would it make sense to have a @todo to make it lazy ?

   No biggie I guess since any request that uses a field formatter will have the entity_manager instantiated anyway. Feel free to ignore :-)

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -27,7 +32,89 @@
   +    if ($this->fieldDefinition->getType() === 'string') {
   

   Might be worth a comment that the link option does not make sense for the other field types the formatter supports.

   Also, for symmetry, we should probably add the same condition on "field type === string" than in settingsForm() & viewElements(). If I manually edit my config to switch the 'entity_link' setting, it should still have no effect on other field types.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -27,7 +32,89 @@
   +      $build['#markup'] = $this->t('Link to the @entity_label', ['@entity_label' => $entity_type->getLabel()]);
   

   The image formatter says "Linked to .." in the summary - let's stay consistent with what's already there ?

   "Link to" is fine in the form since, there, it's about an action, but in the summary it's about a status.

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -38,7 +125,19 @@ public function viewElements(FieldItemListInterface $items) {
   +      if ($this->getSetting('entity_link') && ($entity = $items->getParent()->getValue()) && $entity->hasLinkTemplate('canonical')) {
   

   You can use $entity = $items->getEntity(), it's shorter / clearer :-)

   Also, since this will be the same for all deltas, all of this ('entity_link' setting, link template, entity URL) could be computed once and for all outside the foreach ? (using the link option on a multi-valued string is likely to be a rare use case anyway, but well...)

Clarifying issue summary

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/RawStringFormatter.php
   @@ -12,13 +12,12 @@
   + *   id = "raw_string",
   
   @@ -27,7 +26,7 @@
   +class RawStringFormatter extends FormatterBase {
   

   Actually, "raw" is probably not a great idea, as it kind of hints "non-escaped, non-sanitized", which is not the right message to send for a formatter :-)

   Since this formatter is kind of the simplest one you might think of (just prints the checkPlained value), maybe 'basic_formatter' ?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
   @@ -19,15 +24,95 @@
   +    if ($this->fieldDefinition->getType() === 'string') {
   ...
   +    if ($this->fieldDefinition->getType() === 'string' && $this->getSetting('link_to_entity')) {
   
   @@ -35,10 +120,27 @@ class StringFormatter extends FormatterBase {
   +    if ($this->fieldDefinition->getType() === 'string' && $this->getSetting('link_to_entity') && ($entity = $items->getEntity()) && $entity->hasLinkTemplate('canonical')) {
   

   No need to check for the field type anymore :-)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/RawStringFormatter.php
   @@ -12,13 +12,12 @@
     * @FieldFormatter(
   - *   id = "string",
   + *   id = "raw_string",
     *   label = @Translation("Plain text"),
   

   This will affect existing view display entities if you were using string for string_long fields.

   Just pointing out...

My only other thing was the same field definition check that @yched already pointed out. i think it's good to remove that, so that if you allow your field type for that formatter (formatter alter hook), then it works for that as well.

Really love this feature.
Reviewed code, couldn't fault it.
Manual review screenshots below.

I hate to be difficult - but changing all that config from 'string' to 'basic_string' is going to mean people in contrib land will have to do the same.
Any reason why we couldn't leave it as string and then make the new one 'string_link' ?

If there's a reason, then I think this is rtbc.

Any reason why we couldn't leave it as string and then make the new one 'string_link' ?

So the reasoning was that, for 'string' field type, we want only one formatter which optionally lets you link or not, rather than two formatters (one that links and one that doesn't). That's for consistency with how "link or not" works for image fields : it's an option within the image formatter.

Then it makes sense that this single formatter for the 'string' field type is the formatter that we call 'string', rather than "the 'string' formatter is used by a couple field types but *not* the 'string' field type, which in fact uses a formatter named 'string_link'".

Silly question : would it make sense to have 'string_long' field type also use the 'string' formatter (the one that can link) ?

I kind of like the consistency of :
- 'string' / 'string_long' field types --> 'string' formatter
- 'email' / 'phone' / ... field types -> 'basic_string' formatter
But no strong opinion...

re: #28 - discused with @dawehner, there's no use case for linking on long strings.

Current patch is RTBC as far as I'm concerned :-)

Thanks @yched that makes sense - +1 RTBC from me then

Looks good to me. I'd not have minded keeping the same name, but this is also a tiny update for contrib/custom code to make so I think that's OK given the issue is critical.

Not committing yet just so this has a bit longer than an hour at RTBC.

Will need some kind of change record though for that.

Committed/pushed to 8.0.x, thanks!

Some retroactive D8MI tags.