- Issue created by @brooke_heaton
- Merge request !22Check for internal links before rendering. Convert internal link to full url. β (Closed) created by brooke_heaton
- Assigned to brooke_heaton
- Status changed to Needs review
3 months ago 3:12am 17 September 2024 - πΊπΈUnited States brooke_heaton
Attached patch will convert internal links to full urls. See Merge Request.
- πΊπΈ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 10:04pm 18 September 2024 - Issue was unassigned.
- Status changed to Active
3 months ago 10:15pm 18 September 2024 - πΊπΈ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.
- πΊπΈ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 ...
Automatically closed - issue fixed for 2 weeks with no activity.