Displays route rather than link for internal link in link field

Created on 17 September 2024, 3 months ago

Problem/Motivation

The barcodes module can be used to render a 'link' field as a barcode. I am attempting to render an internal link in a field that allows both 'internal' and 'external' links. It works fine for external links, however when attempting to link to internal content, the output is a route. For example, it links to entity:node/3821, which is NOT a url. Instead, the module should detect if the url is internal or external. If internal, it should render the route to a fully qualified url with https://domain.com/path-to-content

Steps to reproduce

Install the 'barcodes' module. Add a content type that includes a 'link' field. Configure the link field to allow BOTH internal and external links. Set the field display of the link field to 'QR Code'. Add a content item and add an internal link for the link field. View the node and scan the barcode. Note that the link will not work.

Proposed resolution

Change the logic in Drupal\barcodes\Plugin\Field\FieldFormatter to detect if a link is internal or external. If internal, change the value of the field from a Drupal route to a full url. The problem lies in the $value variable in viewElements. This value is never converted to full url for internal links.

Remaining tasks

User interface changes

API changes

Data model changes

πŸ› Bug report
Status

Active

Version

2.1

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States brooke_heaton

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @brooke_heaton
  • Assigned to brooke_heaton
  • Status changed to Needs review 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States brooke_heaton

    Attached patch will convert internal links to full urls. See Merge Request.

  • Pipeline finished with Success
    3 months ago
    Total: 160s
    #284936
  • πŸ‡ΊπŸ‡ΈUnited States tr Cascadia

    Have you tried using the Twig filter instead?

    There are a number of fundamental problems with the FieldFormatter approach.

    The FieldFormatter provided by Barcodes is a way to display the alphanumeric content of a field as a barcode. But this is just a formatter, and the assumption is that the data stored in your field has the correct length and character subset to be displayed as the barcode type you have chosen. The FieldFormatter has no way to ensure that - to do that you would have to have a barcode-type-specific field with all the constraints to control whether that barcode field can hold just numbers, or numbers and letters, or special characters, or even non-ASCII characters. And the field would have to have validation to ensure that the checksums, if any, were present and calculated properly. Etc. Apart from having a dedicated FieldType, with a dedicated FieldFormattter for that type, there is no way to ensure that the data stored in the field can be properly represented by the formatter for that barcode type.

    Likewise, there is an architectural problem in the design of core Drupal FieldFormatters. In principle, if my FieldFormatter can properly display data that can be represented as a string, then I should be able to apply that FieldFormatter to *any* FieldType that has a value that can be represented as a string. But that's not how it currently works - the FieldFormatter has to explicitly name each and every datatype that it knows how to format. So text is different than text_long which is different than email, etc, even though they are all strings and a FieldFormatter that can displays strings should work just fine. This is not a scalable or extensible architecture - it really should be based on inheritance of data types so if I can display the parent type I should be able to display the subtype.

    Specifically, when talking about a LinkItem FieldType, this data is NOT stored as a string. So why does the Barcodes FieldFormatter support LinkItem field types? I don't know - it's historical I suspect, because that feature was present in the D7 version. And it's useful, for sure. But fundamentally, your field isn't storing a string exactly, it is holding a value that can be manipulated into being a string representing a URL. Normally that manipulation would be done by the dedicated LinkFormatter - that contains all the logic and special cases needed to transform the data stored in a LinkItem into an HTML string suitable for display on your website. I would not like to have to re-implement and maintain all that special-case code.

    I am really reluctant to put code into the field formatter which tests for field type and performs extra processing to deal with that field type. It's just not a sustainable strategy.

    This is why I'm suggesting using the barcode Twig filter approach instead. In a template, you have access to the href of the rendered URL, which is produced by the default LinkFormatter. The you can pipe that value into the barcode Twig filter to have it displayed as a barcode. I wrote a little documentation on how to use this at https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... β†’

    Another option to explore would be to use a module like https://www.drupal.org/project/custom_twig_formatter β†’ (which I just found) which provides a field formatter that understands Twig so you can use it to display your LinkItem field with the barcode Twig filter. (I might even steal this idea and use it in the Barcodes FieldFormatter to allow site developers to use Twig to select the data they want to format as a Barcode, rather than just using the "value" of the underlying Field.) And there are also modules like Twig Tools and Twig Tweak which make working with fields in a template a little easier.

    So rather than adding all that link-specific code, I'm inclined to just document one or more methods for using a LinkItem with the Barcodes module, and maybe even then remove "link" from the supported field types for the Barcodes FieldFormatter. The advantage of doing it this way is that these examples of how to use the Barcodes module functionality with additional types of fields are relevant to more than just LinkItem, so this is the basis of a scalable solution for using the Barcodes module with any field type that can be transformed into a string on one way or another.

  • πŸ‡ΊπŸ‡ΈUnited States brooke_heaton

    This is for a link field. Link fields can be configured to include internal or external links. If an internal link is stored, the $value of the field is not a fully rendered URL, it is a Drupal path (e.g. entity:node/3821). When the field is rendered as a barcode, problematically the scanned barcode will render to entity:node/3821. This PR and patch fixes this issue. I would encourage you to review it.

  • πŸ‡ΊπŸ‡ΈUnited States tr Cascadia

    Well you don't have to leave in a huff. I was just explaining the limitations of using a FieldFormatter, hopefully helping you understand *why* the Link field has a problem (because it doesn't store a string) and writing all this down as a prelude to documenting this properly.

    Using the Barcode Twig filter provided by this module is a simple way to accomplish what you want, and that feature exists now. That's why this module provides a Block, a FieldFormatter, and a Twig filter, because just one feature doesn't fit all use cases.

    As I said, this issue is new to me, since I didn't write this module, so I am just thinking out loud to explore possibilities for how to do this properly going forward. Writing the documentation and providing an example, like I said I would do, should go a long way to help people with different use cases. And using the idea from custom_twig_template, which I said I would explore, will make the Barcodes FieldFormatter much more useful.

  • Status changed to Closed: works as designed 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States brooke_heaton
  • Issue was unassigned.
  • Status changed to Active 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States tr Cascadia

    Leaving it open as an issue so I can work on the examples and documentation. Of course, you could also contribute ...

  • πŸ‡ΊπŸ‡ΈUnited States tr Cascadia

    In Drupal 7, it appears that support for the link field was added by this piece of code:

            if (!isset($item['value'])) {
              $value = $item['url'];
            }
            else {
              $value = $item['value'];
            }

    What this does is check to see if the field has a 'value'. If it does, encode that value as a barcode. If it doesn't, then assume that there is a 'url' and encode that 'url' as a barcode. In practice, the only field type that doesn't have a 'value' is the link field, and in D7 the link field had a 'url' which was a fully-resolved URL string. So this worked in D7, but is a bit of a hack because it makes implicit assumptions. (Which BTW turn out not to hold in D8+ ...).

    In D8+, this was replaced by:

        if ($item->mainPropertyName()) {
          $value = $item->__get($item->mainPropertyName());
        }
        else {
          $value = $item->getValue();
        }

    This is better code, because in D8+ if a field doesn't have a 'value' it has to specify a mainPropertyName(). However, it is still a bit of a hack because it makes the assumption that the value stored in the property named mainPropertyName() is in usable form. In the case of a link field, that mainPropertyName() is a 'uri' datatype and a URI is stored in it, not a URL. The scheme can be internal: and still be a valid URI, but this is not suitable for formatting as a barcode anymore.
    Core Views also has similar code (\Drupal\views\Plugin\views\field\EntityField):

          // Find the value using the main property of the field. If no main
          // property is provided fall back to 'value'.
          elseif ($main_property_name = $field_item->mainPropertyName()) {
            $values[] = $field_item->{$main_property_name};
          }
          else {
            $values[] = $field_item->value;
          }

    Apart from the syntax, this is exactly what Barcodes currently does to get the field value.

    The missing part in the Field API seems to be, once I have the mainPropertyName(), how do I get the processed value? Normally this would be through a getter function. And indeed there is a getUrl() method on the link field that I think will return what you need. But there is no association between the 'uri' main property name and the getUrl() method as a way to retrieve that value. So programmatic methods like the above of figuring out what value we should encode fail in the specific case of the link field using a non-http scheme, but not in the case of any other field type.

    So one thing we can do here is just enhance this one little piece of code to look like this:

        if ($item instanceof \Drupal\link\LinkItemInterface) {
          // Always want URLs encoded as barcodes to be absolute.
          $item->options += ['absolute' => TRUE];
          $value = $item->getUrl()->toString();
        }
        elseif ($item->mainPropertyName()) {
          $value = $item->__get($item->mainPropertyName());
        }
        else {
          $value = $item->getValue();
        }

    This makes an exception for Link, then the elseif case is an exception for any other field that may not use 'value', then the fallback case is to just use the value like with a normal field.

    This is still a hack, but I like it better because it's a minimal hack to an internal protected method which was written specifically to hold this sort of hack. And the main viewElement() method doesn't have to be touched.

    Regardless, I think it would be even better to have a general way to deal with any other field that people want to support, and to keep on adding to the list of hacks is not the way.

  • πŸ‡ΊπŸ‡ΈUnited States tr Cascadia

    Now that tests have been written to test the barcodes field formatter on all supported fields (see πŸ“Œ Add tests for field types Active ), I am opening a new MR here to add a new test case for internal links to demonstrate it fails, then to add the fix from #9 to show that it properly addresses this failure.

  • Merge request !262.1.x β†’ (Closed) created by tr
  • Merge request !27Resolve #3474820 "Displays route rather" β†’ (Merged) created by tr
  • πŸ‡ΊπŸ‡ΈUnited States tr Cascadia

    As you can see, the test-only patch demonstrates the failure described in the original post. Now to add the patch from #9 ...

  • πŸ‡ΊπŸ‡ΈUnited States tr Cascadia

    et voilΓ 

  • Pipeline finished with Skipped
    23 days ago
    #349777
    • tr β†’ committed 3276721e on 2.1.x
      Issue #3474820 by tr: Displays route rather than link for internal link...
  • πŸ‡ΊπŸ‡ΈUnited States tr Cascadia
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024