Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.5] Fix for URL field formatter not able to render relative URLs properly. #55702

Closed
wants to merge 3 commits into from

Conversation

mike-treadway
Copy link

@mike-treadway mike-treadway commented Jan 23, 2020

Summary

Fields using a URL fielder formatter with a relative URL were not rendering correctly because the base URL information in Kibana was not being passed down into the formatter. This caused the formatter to not understand how to derive the full URL and instead just returned a with the URL text inside.

Checklist

- [ ] 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

- [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

@kibanamachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@mike-treadway
Copy link
Author

mike-treadway commented Jan 23, 2020

Also, I did notice that the code in master fixes this issue in a similar way (allowing the field formatter to get the default base URL info). My hope is that this PR could be put in for v7.x and not worry about carrying this bug fix forward into v8.

@legrego
Copy link
Member

legrego commented Jan 31, 2020

Hi @mike-treadway, thanks for your contribution, and thanks also for your patience. It looks like we missed the window for the last 7.5.x release, so we may have to point this against the 7.x branch instead to get it into a 7.6.x patch release.

I'm surprised to find this fixed in master and not in 7.x, but I'm honestly not familiar enough with field formatters to say why that's the case.

I am going to ping the @elastic/kibana-app-arch team to help you out from here. They'll review and provide guidance going forward.

@legrego legrego requested a review from a team January 31, 2020 14:29
@legrego legrego added Feature:FieldFormatters Team:Visualizations Visualization editors, elastic-charts and infrastructure Team:AppArch and removed Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jan 31, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lukeelmers
Copy link
Member

Thanks @mike-treadway! Really appreciate you taking the time to put up this PR. Looks like this is in relation to our discussion over in #35235

The fix that you saw on master should also be in place in 7.x (for 7.7), and 7.6 branches. (Here's the PR where it was backported to 7.x, which was later cut to 7.6).

Currently we haven't planned a 7.5.3 release, as 7.5.2 was released a little over a week ago, and we are expecting 7.6 to be released sometime in the near future.

If you'd like, we could go through a review process on this to merge a fix directly to 7.5 just in case there ends up being a 7.5.3 release, at which point it would get picked up. But I also wouldn't want to waste your time working on something that may never be released.

What are your thoughts? Are you able to confirm that everything works for you as expected on the 7.6 branch? If that's the case, it may be worth waiting until 7.6 releases, as that shouldn't be too far off.

@timroes timroes removed the request for review from a team February 3, 2020 12:50
@lukeelmers
Copy link
Member

@mike-treadway I'm going to go ahead and close this PR as stale since there hasn't been any activity for quite some time. If you'd still like to contribute, feel free to re-open & we can pick the conversation back up. Thanks for your contribution!

@lukeelmers lukeelmers closed this Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants