Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#54372 new defect (bug)

class-wp-automatic-updater.php adds a period to home_url()

Reported by: chris554's profile chris554 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version:
Component: Upgrade/Install Keywords: has-patch
Focuses: administration Cc:

Description

The automatic emails that are generated by class-wp-automatic-updater.php have an issue where a period character immediately follows a home_url() string in the body of the message. When using a third-party mailing and/or link-branding service on these emails, those periods are being incorporated into the links, rendering them useless.

Example body of an update email:
Howdy! Some plugins and themes have automatically updated to their latest versions on your site at https://www.example.com. No further action is needed on your part.

When passed through a service that brands the links, such as MailChimp or SendGrid, that URL is interpreted as "https://www.example.com.", and not as "https://www.example.com"

The code for this is reused through the file, but an example starts at line 1031:

<?php
                switch ( $type ) {
                                case 'success':
                                        if ( $successful_plugins && $successful_themes ) {
                                                /* translators: %s: Site title. */
                                                $subject = __( '[%s] Some plugins and themes have automatically updated' );
                                                $body[]  = sprintf(
                                                        /* translators: %s: Home URL. */
                                                        __( 'Howdy! Some plugins and themes have automatically updated to their latest versions on your site at %s. No further action is needed on your part.' ),
                                                        home_url()
                                                ); 

You can see the sprintf() string in the $body[] section has "%s." which renders as "home_url().", which renders to https://www.example.com. leading to the period being interpreted as part of the link.

This is present at lines:
1038
1046
1054
1067
1075
1083

Suggested fix: add a space between the %s and the period, or eliminate the period from the string entirely. Either would fix the issue with minimal changes.

I would submit a change myself but I'm at the absolute limit of my coding abilities here, just finding what sent the email and why the period was there took all my effort.

I realize that this is a minor bug, but it does render the links in the update emails un-clickable, a real hassle when I have 20+ sites emailing me and I'm trying to quickly click to the one with the issue.

Attachments (2)

54372.earlier-location.diff (3.0 KB) - added by costdev 3 years ago.
Move on your site at %s to an earlier location in the message.
54372.brackets.diff (3.0 KB) - added by costdev 3 years ago.
Wrap %s in brackets: ( %s ).

Download all attachments as: .zip

Change History (10)

#1 in reply to: ↑ description @chris554
3 years ago

Sorry to be precise that's

wp-admin/includes/class-wp-automatic-updater.php

in trunk

#2 @SergeyBiryukov
3 years ago

  • Component changed from Administration to Upgrade/Install

@costdev
3 years ago

Move on your site at %s to an earlier location in the message.

@costdev
3 years ago

Wrap %s in brackets: ( %s ).

#3 @costdev
3 years ago

  • Keywords reporter-feedback has-patch added; needs-patch removed

Hi @chris554! Thanks for opening this ticket and welcome to Trac!

If possible, can you provide us with the steps required to reproduce this issue?

More specifically:

  • Have you used a particular plugin or feature to pass the emails through a service that brands the links?
    • If so, can you provide the name and let us know whether or not you have made any changes to the default options of the plugin/feature? Thanks!
  • If not, how are you passing the emails to the service?

Adding reporter-feedback to get some information/instructions in order to reproduce the issue.


Patches 54372.earlier-location.diff and 54372.brackets.diff offer two possible solutions.

Should this issue be verified by testers and the ticket accepted, it should have needs-copy-review and needs-testing keywords applied.

Adding has-patch.

#4 @chris554
3 years ago

Hi @costdev, thank you for the amazingly quick response.

The plugin I'm using for SMTP emails is "WP Mail SMTP", and within that plugin the only configuration is adding my API key to their built-in integration with SendGrid. My understanding is that WP Mail SMTP grabs all outgoing emails and sends them via API call to SendGrid, which handles mail delivery and link branding, etc.

I had opened a ticket with SendGrid prior to this ticket, and they confirmed that on their end the JSON received from WP Mail SMTP contained the period as part of the URL, closing the ticket on their end. It only affected that one specific place where the home_url() is followed by a period, all the other links in the emails have spaces preceding and following.

I think, personally, the 54372.earlier-location.diff is a cleaner fix.

Still new to this process, is there anything I should do to test this, other than just changing the wp-admin/includes/class-wp-automatic-updater.php files on my site to the changed ones?

#5 @chris554
3 years ago

  • Keywords reporter-feedback removed

Forgot to remove the feedback tag

#6 @costdev
3 years ago

@chris554 Thanks for providing more information about your setup!

The patches above are only to help speed up the process should this discussion lead to a change in Core. Personally, I think the onus is on the email service to parse the URLs accurately.

Based on the issue, I assume that their parsing amounts to:

  1. Split the email message into 'parts' when a space is found.
  2. If http:// or https:// is detected, link this 'part' as-is.

Scroll to the very bottom of this example to see how a service could parse the home_url() more accurately. Disclaimer: This is an illustrative example. Do not use in production.

This ticket was mentioned in Slack in #core by costdev. View the logs.


3 years ago

#8 @audrasjb
3 years ago

  • Version trunk deleted

Hello,
It appears this issue wasn't introduced in WP 5.9, so I'm removing the trunk version from the ticket.

Note: See TracTickets for help on using tickets.