Fix rendering of formatted value

[gospodarsky]

Summary

To #35235

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• This was checked for cross-browser compatibility, including a check against IE11
• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately
• This includes a feature addition or change that requires a release note and was labeled appropriately

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 91b9570

History

• 💔 Build #15388 failed a724e3e

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[alexwizp]

@elasticmachine merge upstream

[alexwizp]

@elasticmachine merge upstream

[elasticmachine]

merge conflict between base and head

[lukeelmers]

Tested and excited to confirm that it works! However, I do think we need to rearrange some of these items. I tend to agree with @alexwizp that the modifications to htmlConvert make it less intuitive, and I actually think the problem lies in a different place...

From what I can tell, the problem is not that we need to set the url params on the field formatter class and then override them later, it's that in the pipeline_helpers/utilities we are expecting the format.convert() on line 171 to accept 4 arguments like they do in index_patterns/format_hit.

If you add a debugger to the html content type here and compare the same results in Discover and Visualize/Dashboards, you will see what I'm talking about. Discover doesn't use the pipeline_helpers/utilities, and instead the formatter getConverterFor is called directly from the index patterns via format_hit, and all 4 args are preserved. But pipeline_helpers/utilities uses convert directly, and then the parsedUrl that is provided does not actually get passed all the way through.

So while the proposed changes here do technically fix the problem, they are really just a workaround. I see two possible ways to approach this:

1. We can change pipeline_helpers/utilities to use getConverterFor instead of convert (one-line change; I tested this locally and it works.
2. We fix the FieldFormat convert method so that it accepts more args and applies them correctly (i.e. matching the HtmlContextTypeConvert type).

Option 2 feels like the "right" answer to me since it would fix the problem for anybody using convert... But this also makes it a bit more risky and would require careful testing. I'm also open to Option 1, though if we went that route it would be good to document this as a follow-up task to investigate later. Curious to hear your thoughts on this.

Lastly, we should definitely add a functional test for this specific case. Really all this needs to do is adjust one of the fields in the index pattern to be url-based (perhaps IP field since that's what was originally presented in the parent issue for this), then navigate to both discover and visualize and verify that the relevant html element is in fact a link. The reason I think a functional test is crucial is because this is a very specific feature that I know does not get tested manually very often, and could easily regress in the future. The field formatter pipeline utilities are also in need of more unit tests in general, but I think a functional test will suffice for this particular issue.

[alexwizp]

see comments above

[alexwizp]

Approved by mistake

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request
• Commit: 25cba4e

History

• 💔 Build #16383 failed 790dcd5
• 💔 Build #16359 failed 6eb5fa0
• 💔 Build #16142 failed 3d2823f
• 💔 Build #16126 failed 0a64043
• 💔 Build #16107 failed 46c7197

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[gospodarsky]

Resolved in #53265