-
Notifications
You must be signed in to change notification settings - Fork 798
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
Edited error message #13507
Edited error message #13507
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: November 5, 2019. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend linking to the Debug page with the site URL already filled in. I think you could also remove the comment at the end of the line now.
@jeherve made the change. The debug link will now include |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The backticks aren't going to work well here. I'd recommend building the string using sprintf
and wp_kses
to ensure that translators cannot break the string while translating.
@jeherve That's great advice. While I knew a little bit about this in theory, I learned so much more in practice while making changes and getting this to work. What do you think about the latest commit? |
sprintf( | ||
/* translators: 1st placeholder is a link to Jetpack debug page, 2nd one fetches current site URL. */ | ||
__( | ||
'<a href="%1$s%2$s">Visit the Jetpack.com debug page</a> for more information or <a href="https://jetpack.com/contact-support/">contact support</a>.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to do this with just one placeholder.
'<a href="%1$s%2$s">Visit the Jetpack.com debug page</a> for more information or <a href="https://jetpack.com/contact-support/">contact support</a>.', | ||
'jetpack' | ||
), | ||
'https://jetpack.com/support/debug/?url=', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do just the one place holder, we condense these two into arguments (the debug URL and the site URL) into one:
esc_url( add_query_arg( 'url', urlencode( site_url() ), 'https://jetpack.com/support/debug/' ) )
This also ensures we're using the right escaping functions (esc_url()
for the whole URL, urlencode()
for the query string parameter) in the right places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! didn't think of that. Also add_query_arg
is a TIL for me. Thanks. Made the change now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. However, while it works well under Tools > Site Health, we can't really display links in Jetpack > Debug right now:
You'd need to ensure that the output there allows for links, maybe using wp_kses
as well:
echo '<p class="jetpack-test-details">' . esc_html( $fail['resolution'] ) . '</p>'; |
@jeherve Good point. Thanks for testing. Made the change (also resolved the conflict with the master branch) and this looks good to go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. It should be good to merge. I only wonder if it may be best to have those links open in a new tab, since they direct you out of wp-admin.
@jeherve You're right, opening the link in a new tab makes sense. Let me make that change.. |
@jeherve All done. Ready to merge 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good addition. I'd have 2 remarks:
- Now that you've added a new attribute to that HTML tag (
target
), you'll want to update the list of attributes you're allowing inwp_kses
. If you don't,wp_kses
will strip them for you. - When you are adding a
target="_blank"
attribute, you also want to add arel="noopener noreferrer"
attribute to avoid the problems listed here: https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/ https://stackoverflow.com/a/50709760/1045686
Latest commit includes cc: @jeherve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Merging.
One thing I'd want to check is the Ideally, in the PR description and testing instructions, there would be enough detail to recreate the error state so it can be tested without figuring out how to break JP. |
Noted. I'm still learning but I'll remember this for the next time. I wanted to recreate the error message to test things and Jeremy suggested the simplest way is to put it outside of if/else block. That's what I did. I don't know if there is a better way to recreate the error. Can you share some details on how you did it? @kraftbj Edit: Wait I think you did it via |
* 7.9: Changelog * Update version number * Update stable tag and tested up to * Changelog: add #13530 * changelog: add #13578 * Changelog: add #13598 * Changelog: add entry for numerous block preview changes * Changelog: add #13599 * changelog: add #13541 * Changelog: add #13542 * Changelog: add #13331 * Changelog: add #13558 * Changelog: add #13409 * Changelog: add #13582 * Changelog: add #13600 * Changelog: add #13601 * Changelog: add #13595 * Changelog: add #12695 * Changelog: add #13009 * Changelog: add #13649 * Changelog: add #13450 * Changelog: add #13507 * Changelog: add #13658 * Changelog: add #13687 * changelog: add #13683 * Changelog: add #9323 * Changelog: add #13681 * Fix typos in readme * Add link to WordPress Beta Tester plugin * Changelog: add #13630 * Changelog: add #13695 * Changelog: add #13659 * Changelog: add #13716 * Changelog: add #13664 * Changelog: add #13682 * Changelog: add #13362 * Changelog: add #13563 * Add testing list for #13563 * Changelog: add #13735 * Changelog: add #13752 * Changelog: add #13624 * Changelog: add #13756 * Changelog: add #13745 * Changelog: add #13728 * Changelog: add #13779 * Changelog: add #13699 * Changelog: add #13804 * Changelog: add #13761 * Changelog: add #13637 * Changelog: add #13517 * Changelog: add #13521 * Changelog: add #13729 * Testing list: add testing instructions for #13729 * Changelog: add sync changes * Changelog: add #13807 * Changelog: add #13654 * Changelog: add #13795 * Changelog: add #13801 * Changelog: add #13818 * Changelog: add #13725 * Changelog: add #13831 * Changelog: add #13516 * Testing list: add Twenty Twenty instructions * Changelog: add #13799 * Changelog: add #13805 * Changelog: add #13688 * Changelog: add #13830
Fixes #13115
Changes proposed in this Pull Request:
Testing instructions:
Proposed changelog entry for your changes: